* [PATCH 1/5] Modules/clone: fix double close()
@ 2018-11-07 13:04 Kamil Dudka
2018-11-07 13:04 ` [PATCH 2/5] Zle/computil: do not use strcpy() for overlapping blocks Kamil Dudka
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Kamil Dudka @ 2018-11-07 13:04 UTC (permalink / raw)
To: zsh-workers
Detected by Coverity Analysis.
Error: USE_AFTER_FREE (CWE-825):
zsh-5.5.1/Src/Modules/clone.c:71: closed_arg: "close(int)" closes "ttyfd".
zsh-5.5.1/Src/Modules/clone.c:99: double_close: Calling "close(int)" closes handle "ttyfd" which has already been closed.
97| setsparam("TTY", ztrdup(ttystrname));
98| }
99|-> close(ttyfd);
100| if (pid < 0) {
101| zerrnam(nam, "fork failed: %e", errno);
---
Src/Modules/clone.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Src/Modules/clone.c b/Src/Modules/clone.c
index ef6275dcf..4b2655505 100644
--- a/Src/Modules/clone.c
+++ b/Src/Modules/clone.c
@@ -96,7 +96,8 @@ bin_clone(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
init_io(NULL);
setsparam("TTY", ztrdup(ttystrname));
}
- close(ttyfd);
+ else
+ close(ttyfd);
if (pid < 0) {
zerrnam(nam, "fork failed: %e", errno);
return 1;
--
2.17.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/5] Zle/computil: do not use strcpy() for overlapping blocks
2018-11-07 13:04 [PATCH 1/5] Modules/clone: fix double close() Kamil Dudka
@ 2018-11-07 13:04 ` Kamil Dudka
2018-11-07 13:04 ` [PATCH 3/5] Src/exec: avoid fd leak on fork() failure Kamil Dudka
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Kamil Dudka @ 2018-11-07 13:04 UTC (permalink / raw)
To: zsh-workers
Detected by Coverity:
Error: BUFFER_SIZE (CWE-120):
zsh-5.5.1/Src/Zle/computil.c:564: overlapping_buffer: The source buffer "str->str + 2" potentially overlaps with the destination buffer "str->str", which results in undefined behavior for "strcpy".
zsh-5.5.1/Src/Zle/computil.c:564: remediation: Replace "strcpy(dest, src)" with "memmove(dest, src, strlen(src)+1)".
562| str->str = ztrdup(str->str);
563| if (hide[1] && str->str[0] == '-' && str->str[1] == '-')
564|-> strcpy(str->str, str->str + 2);
565| else if (str->str[0] == '-' || str->str[0] == '+')
566| strcpy(str->str, str->str + 1);
Error: BUFFER_SIZE (CWE-120):
zsh-5.5.1/Src/Zle/computil.c:566: overlapping_buffer: The source buffer "str->str + 1" potentially overlaps with the destination buffer "str->str", which results in undefined behavior for "strcpy".
zsh-5.5.1/Src/Zle/computil.c:566: remediation: Replace "strcpy(dest, src)" with "memmove(dest, src, strlen(src)+1)".
564| strcpy(str->str, str->str + 2);
565| else if (str->str[0] == '-' || str->str[0] == '+')
566|-> strcpy(str->str, str->str + 1);
567| }
568| }
---
Src/Zle/computil.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 5526e0ad0..cb1c01042 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -561,9 +561,9 @@ cd_init(char *nam, char *hide, char *mlen, char *sep,
if (str->str == str->match)
str->str = ztrdup(str->str);
if (hide[1] && str->str[0] == '-' && str->str[1] == '-')
- strcpy(str->str, str->str + 2);
+ memmove(str->str, str->str + 2, strlen(str->str) - 1);
else if (str->str[0] == '-' || str->str[0] == '+')
- strcpy(str->str, str->str + 1);
+ memmove(str->str, str->str + 1, strlen(str->str));
}
}
for (ap = args; *args &&
--
2.17.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/5] Src/exec: avoid fd leak on fork() failure
2018-11-07 13:04 [PATCH 1/5] Modules/clone: fix double close() Kamil Dudka
2018-11-07 13:04 ` [PATCH 2/5] Zle/computil: do not use strcpy() for overlapping blocks Kamil Dudka
@ 2018-11-07 13:04 ` Kamil Dudka
2018-11-07 13:04 ` [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs() Kamil Dudka
2018-11-07 13:04 ` [PATCH 5/5] Src/utils: fix memory leaks in mailstat() Kamil Dudka
3 siblings, 0 replies; 7+ messages in thread
From: Kamil Dudka @ 2018-11-07 13:04 UTC (permalink / raw)
To: zsh-workers
Detected by Coverity Analysis:
Error: RESOURCE_LEAK (CWE-772):
zsh-5.5.1/Src/exec.c:4680: open_fn: Returning handle opened by "open".
zsh-5.5.1/Src/exec.c:4680: var_assign: Assigning: "fd" = handle returned from "open(nam, 449, 384)".
zsh-5.5.1/Src/exec.c:4810: leaked_handle: Handle variable "fd" going out of scope leaks the handle.
4808| /* fork or open error */
4809| child_unblock();
4810|-> return nam;
4811| } else if (pid) {
4812| int os;
---
Src/exec.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Src/exec.c b/Src/exec.c
index c4a2740c0..042ba065a 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4805,7 +4805,8 @@ getoutputfile(char *cmd, char **eptr)
}
if ((cmdoutpid = pid = zfork(NULL)) == -1) {
- /* fork or open error */
+ /* fork error */
+ close(fd);
child_unblock();
return nam;
} else if (pid) {
--
2.17.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs()
2018-11-07 13:04 [PATCH 1/5] Modules/clone: fix double close() Kamil Dudka
2018-11-07 13:04 ` [PATCH 2/5] Zle/computil: do not use strcpy() for overlapping blocks Kamil Dudka
2018-11-07 13:04 ` [PATCH 3/5] Src/exec: avoid fd leak on fork() failure Kamil Dudka
@ 2018-11-07 13:04 ` Kamil Dudka
2018-11-07 14:21 ` Mikael Magnusson
2018-11-07 13:04 ` [PATCH 5/5] Src/utils: fix memory leaks in mailstat() Kamil Dudka
3 siblings, 1 reply; 7+ messages in thread
From: Kamil Dudka @ 2018-11-07 13:04 UTC (permalink / raw)
To: zsh-workers
Detected by Coverity Analysis:
Error: USE_AFTER_FREE (CWE-825):
zsh-5.5.1/Src/module.c:1390: freed_arg: "deletemathfunc" frees "f".
zsh-5.5.1/Src/module.c:1352:6: freed_arg: "zfree" frees parameter "f".
zsh-5.5.1/Src/mem.c:1888:5: freed_arg: "free" frees parameter "p".
zsh-5.5.1/Src/module.c:1394: deref_after_free: Dereferencing freed pointer "f".
1392| ret = 1;
1393| } else {
1394|-> f->flags &= ~MFF_ADDED;
1395| }
1396| }
---
Src/module.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/Src/module.c b/Src/module.c
index 4ae78310f..33d75ebbd 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -1390,8 +1390,6 @@ setmathfuncs(char const *nam, MathFunc f, int size, int *e)
if (deletemathfunc(f)) {
zwarnnam(nam, "math function `%s' already deleted", f->name);
ret = 1;
- } else {
- f->flags &= ~MFF_ADDED;
}
}
f++;
--
2.17.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 5/5] Src/utils: fix memory leaks in mailstat()
2018-11-07 13:04 [PATCH 1/5] Modules/clone: fix double close() Kamil Dudka
` (2 preceding siblings ...)
2018-11-07 13:04 ` [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs() Kamil Dudka
@ 2018-11-07 13:04 ` Kamil Dudka
3 siblings, 0 replies; 7+ messages in thread
From: Kamil Dudka @ 2018-11-07 13:04 UTC (permalink / raw)
To: zsh-workers
Detected by Coverity Analysis:
Error: RESOURCE_LEAK (CWE-772):
zsh-5.5.1/Src/utils.c:7406: alloc_fn: Storage is returned from allocation function "appstr".
zsh-5.5.1/Src/string.c:200:5: alloc_fn: Storage is returned from allocation function "realloc".
zsh-5.5.1/Src/string.c:200:5: identity_transfer: Passing "realloc(base, strlen(base) + strlen(append) + 1UL)" as argument 1 to function "strcat", which returns that argument.
zsh-5.5.1/Src/string.c:200:5: return_alloc_fn: Directly returning storage allocated by "strcat".
zsh-5.5.1/Src/utils.c:7406: var_assign: Assigning: "dir" = storage returned from "appstr(ztrdup(path), "/cur")".
zsh-5.5.1/Src/utils.c:7407: noescape: Resource "dir" is not freed or pointed-to in "stat".
zsh-5.5.1/Src/utils.c:7407: leaked_storage: Variable "dir" going out of scope leaks the storage it points to.
7405| /* See if cur/ is present */
7406| dir = appstr(ztrdup(path), "/cur");
7407|-> if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
7408| st_ret.st_atime = st_tmp.st_atime;
7409|
Error: RESOURCE_LEAK (CWE-772):
zsh-5.5.1/Src/utils.c:7412: alloc_fn: Storage is returned from allocation function "appstr".
zsh-5.5.1/Src/string.c:200:5: alloc_fn: Storage is returned from allocation function "realloc".
zsh-5.5.1/Src/string.c:200:5: identity_transfer: Passing "realloc(base, strlen(base) + strlen(append) + 1UL)" as argument 1 to function "strcat", which returns that argument.
zsh-5.5.1/Src/string.c:200:5: return_alloc_fn: Directly returning storage allocated by "strcat".
zsh-5.5.1/Src/utils.c:7412: var_assign: Assigning: "dir" = storage returned from "appstr(dir, "/tmp")".
zsh-5.5.1/Src/utils.c:7413: noescape: Resource "dir" is not freed or pointed-to in "stat".
zsh-5.5.1/Src/utils.c:7413: leaked_storage: Variable "dir" going out of scope leaks the storage it points to.
7411| dir[plen] = 0;
7412| dir = appstr(dir, "/tmp");
7413|-> if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
7414| st_ret.st_mtime = st_tmp.st_mtime;
7415|
Error: RESOURCE_LEAK (CWE-772):
zsh-5.5.1/Src/utils.c:7418: alloc_fn: Storage is returned from allocation function "appstr".
zsh-5.5.1/Src/string.c:200:5: alloc_fn: Storage is returned from allocation function "realloc".
zsh-5.5.1/Src/string.c:200:5: identity_transfer: Passing "realloc(base, strlen(base) + strlen(append) + 1UL)" as argument 1 to function "strcat", which returns that argument.
zsh-5.5.1/Src/string.c:200:5: return_alloc_fn: Directly returning storage allocated by "strcat".
zsh-5.5.1/Src/utils.c:7418: var_assign: Assigning: "dir" = storage returned from "appstr(dir, "/new")".
zsh-5.5.1/Src/utils.c:7419: noescape: Resource "dir" is not freed or pointed-to in "stat".
zsh-5.5.1/Src/utils.c:7419: leaked_storage: Variable "dir" going out of scope leaks the storage it points to.
7417| dir[plen] = 0;
7418| dir = appstr(dir, "/new");
7419|-> if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
7420| st_ret.st_mtime = st_tmp.st_mtime;
7421|
---
Src/utils.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/Src/utils.c b/Src/utils.c
index 914e30c5c..e43a3cdb4 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -7459,19 +7459,28 @@ mailstat(char *path, struct stat *st)
/* See if cur/ is present */
dir = appstr(ztrdup(path), "/cur");
- if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
+ if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) {
+ zsfree(dir);
+ return 0;
+ }
st_ret.st_atime = st_tmp.st_atime;
/* See if tmp/ is present */
dir[plen] = 0;
dir = appstr(dir, "/tmp");
- if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
+ if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) {
+ zsfree(dir);
+ return 0;
+ }
st_ret.st_mtime = st_tmp.st_mtime;
/* And new/ */
dir[plen] = 0;
dir = appstr(dir, "/new");
- if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) return 0;
+ if (stat(dir, &st_tmp) || !S_ISDIR(st_tmp.st_mode)) {
+ zsfree(dir);
+ return 0;
+ }
st_ret.st_mtime = st_tmp.st_mtime;
#if THERE_IS_EXACTLY_ONE_MAILDIR_IN_MAILPATH
@@ -7483,6 +7492,7 @@ mailstat(char *path, struct stat *st)
st_tmp.st_atime == st_new_last.st_atime &&
st_tmp.st_mtime == st_new_last.st_mtime) {
*st = st_ret_last;
+ zsfree(dir);
return 0;
}
st_new_last = st_tmp;
--
2.17.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs()
2018-11-07 13:04 ` [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs() Kamil Dudka
@ 2018-11-07 14:21 ` Mikael Magnusson
2018-11-07 14:35 ` Kamil Dudka
0 siblings, 1 reply; 7+ messages in thread
From: Mikael Magnusson @ 2018-11-07 14:21 UTC (permalink / raw)
To: Kamil Dudka; +Cc: zsh-workers
On 11/7/18, Kamil Dudka <kdudka@redhat.com> wrote:
> Detected by Coverity Analysis:
>
> Error: USE_AFTER_FREE (CWE-825):
> zsh-5.5.1/Src/module.c:1390: freed_arg: "deletemathfunc" frees "f".
> zsh-5.5.1/Src/module.c:1352:6: freed_arg: "zfree" frees parameter "f".
> zsh-5.5.1/Src/mem.c:1888:5: freed_arg: "free" frees parameter "p".
> zsh-5.5.1/Src/module.c:1394: deref_after_free: Dereferencing freed pointer
> "f".
> 1392| ret = 1;
> 1393| } else {
> 1394|-> f->flags &= ~MFF_ADDED;
> 1395| }
> 1396| }
> ---
> Src/module.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/Src/module.c b/Src/module.c
> index 4ae78310f..33d75ebbd 100644
> --- a/Src/module.c
> +++ b/Src/module.c
> @@ -1390,8 +1390,6 @@ setmathfuncs(char const *nam, MathFunc f, int size,
> int *e)
> if (deletemathfunc(f)) {
> zwarnnam(nam, "math function `%s' already deleted", f->name);
> ret = 1;
> - } else {
> - f->flags &= ~MFF_ADDED;
> }
> }
> f++;
> --
> 2.17.2
In the other branch, if f was already deleted, how can we use f->name there?
--
Mikael Magnusson
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs()
2018-11-07 14:21 ` Mikael Magnusson
@ 2018-11-07 14:35 ` Kamil Dudka
0 siblings, 0 replies; 7+ messages in thread
From: Kamil Dudka @ 2018-11-07 14:35 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: zsh-workers
On Wednesday, November 7, 2018 3:21:31 PM CET Mikael Magnusson wrote:
> On 11/7/18, Kamil Dudka <kdudka@redhat.com> wrote:
> > Detected by Coverity Analysis:
> >
> > Error: USE_AFTER_FREE (CWE-825):
> > zsh-5.5.1/Src/module.c:1390: freed_arg: "deletemathfunc" frees "f".
> > zsh-5.5.1/Src/module.c:1352:6: freed_arg: "zfree" frees parameter "f".
> > zsh-5.5.1/Src/mem.c:1888:5: freed_arg: "free" frees parameter "p".
> > zsh-5.5.1/Src/module.c:1394: deref_after_free: Dereferencing freed pointer
> > "f".
> > 1392| ret = 1;
> > 1393| } else {
> > 1394|-> f->flags &= ~MFF_ADDED;
> > 1395| }
> > 1396| }
> > ---
> >
> > Src/module.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/Src/module.c b/Src/module.c
> > index 4ae78310f..33d75ebbd 100644
> > --- a/Src/module.c
> > +++ b/Src/module.c
> > @@ -1390,8 +1390,6 @@ setmathfuncs(char const *nam, MathFunc f, int size,
> > int *e)
> >
> > if (deletemathfunc(f)) {
> >
> > zwarnnam(nam, "math function `%s' already deleted", f->name);
> > ret = 1;
> >
> > - } else {
> > - f->flags &= ~MFF_ADDED;
> >
> > }
> >
> > }
> > f++;
> >
> > --
> > 2.17.2
>
> In the other branch, if f was already deleted, how can we use f->name there?
Do you mean deleted by the call of deletemathfunc(f) from setmathfuncs()?
If deletemathfunc(f) returns -1, it did not delete anything.
If deletemathfunc(f) returns 0, it either called free(f) or already executed:
f->flags &= ~MFF_ADDED
Executing it in setmathfuncs() again could hardly do anything useful.
It would only cause use-after-free in case f->module was true.
Kamil
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-07 14:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 13:04 [PATCH 1/5] Modules/clone: fix double close() Kamil Dudka
2018-11-07 13:04 ` [PATCH 2/5] Zle/computil: do not use strcpy() for overlapping blocks Kamil Dudka
2018-11-07 13:04 ` [PATCH 3/5] Src/exec: avoid fd leak on fork() failure Kamil Dudka
2018-11-07 13:04 ` [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs() Kamil Dudka
2018-11-07 14:21 ` Mikael Magnusson
2018-11-07 14:35 ` Kamil Dudka
2018-11-07 13:04 ` [PATCH 5/5] Src/utils: fix memory leaks in mailstat() Kamil Dudka
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).