On Thu, 2024-07-04 at 21:26 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
>
> On Thu, Jul 4, 2024 at 6:33 AM Bastien Nocera <hadess@xxxxxxxxxx>
> wrote:
> >
> > Error: RESOURCE_LEAK (CWE-772): [#def40] [important]
> > bluez-5.76/src/shared/shell.c:1113:3: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1114:4: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1112|
> > 1113| if (wordexp(rl_line_buffer, &w,
> > WRDE_NOCMD))
> > 1114|-> return NULL;
>
> Derr, this is NOCMD has been found...
Still needs parsing *shrug*
>
> > 1115|
> > 1116| matches = menu_completion(default_menu,
> > text, w.we_wordc,
> >
> > Error: RESOURCE_LEAK (CWE-772): [#def42] [important]
> > bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1415:3: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1413| switch (err) {
> > 1414| case WRDE_BADCHAR:
> > 1415|-> return -EBADMSG;
> > 1416| case WRDE_BADVAL:
> > 1417| case WRDE_SYNTAX:
>
> Ok, but where in the documentation of wordexp it is saying that
> we_wordv is left with anything allocated if it fails? Ive assumed if
> it returns an error no argument has been processed, otherwise this is
> sort of misleading and the errors shall be returned by index of the
> word.
There's nothing says it frees wordv on error, and the code shows it
doesn't:
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/wordexp.c;h=a7362ef31b05280001e961c3a630e953110b7397;hb=HEAD#l2203
> > Error: RESOURCE_LEAK (CWE-772): [#def43] [important]
> > bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1418:3: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1416| case WRDE_BADVAL:
> > 1417| case WRDE_SYNTAX:
> > 1418|-> return -EINVAL;
> > 1419| case WRDE_NOSPACE:
> > 1420| return -ENOMEM;
> >
> > Error: RESOURCE_LEAK (CWE-772): [#def44] [important]
> > bluez-5.76/src/shared/shell.c:1412:2: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1420:3: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1418| return -EINVAL;
> > 1419| case WRDE_NOSPACE:
> > 1420|-> return -ENOMEM;
> > 1421| case WRDE_CMDSUB:
> > 1422| if (wordexp(input, &w, 0))
> >
> > Error: RESOURCE_LEAK (CWE-772): [#def45] [important]
> > bluez-5.76/src/shared/shell.c:1422:3: alloc_arg: "wordexp"
> > allocates memory that is stored into "w.we_wordv".
> > bluez-5.76/src/shared/shell.c:1423:4: leaked_storage: Variable "w"
> > going out of scope leaks the storage "w.we_wordv" points to.
> > 1421| case WRDE_CMDSUB:
> > 1422| if (wordexp(input, &w, 0))
> > 1423|-> return -ENOEXEC;
> > 1424| break;
> > 1425| };
> > ---
> > src/shared/shell.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/shared/shell.c b/src/shared/shell.c
> > index 878be140c336..c09d41ee54df 100644
> > --- a/src/shared/shell.c
> > +++ b/src/shared/shell.c
> > @@ -1117,8 +1117,10 @@ static char **shell_completion(const char
> > *text, int start, int end)
> > if (start > 0) {
> > wordexp_t w;
> >
> > - if (wordexp(rl_line_buffer, &w, WRDE_NOCMD))
> > + if (wordexp(rl_line_buffer, &w, WRDE_NOCMD)) {
> > + wordfree(&w);
> > return NULL;
> > + }
> >
> > matches = menu_completion(default_menu, text,
> > w.we_wordc,
> >
> > w.we_wordv[0]);
> > @@ -1421,15 +1423,20 @@ int bt_shell_exec(const char *input)
> > err = wordexp(input, &w, WRDE_NOCMD);
> > switch (err) {
> > case WRDE_BADCHAR:
> > + wordfree(&w);
> > return -EBADMSG;
> > case WRDE_BADVAL:
> > case WRDE_SYNTAX:
> > + wordfree(&w);
> > return -EINVAL;
> > case WRDE_NOSPACE:
> > + wordfree(&w);
> > return -ENOMEM;
> > case WRDE_CMDSUB:
> > - if (wordexp(input, &w, 0))
> > + if (wordexp(input, &w, 0)) {
> > + wordfree(&w);
> > return -ENOEXEC;
> > + }
> > break;
> > };
>
> If we really need to call wordfree regardless then I'd probably have
> a
> function that wraps wordexp and automatically does wordfree on error.
OK.
>
