From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aloka.lostca.se (aloka.lostca.se [IPv6:2a01:4f8:120:624c::2]) by sourceware.org (Postfix) with ESMTPS id A6F8E385701F for ; Tue, 23 Mar 2021 16:08:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A6F8E385701F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=lostca.se Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arjun.is@lostca.se Received: from aloka.lostca.se (aloka [127.0.0.1]) by aloka.lostca.se (Postfix) with ESMTP id EEC564DD9; Tue, 23 Mar 2021 16:08:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=lostca.se; h=date:from:to :cc:subject:message-id:references:mime-version:content-type :in-reply-to; s=howrah; bh=f4SLxrkZ8b4y/J4PG4SobevLBXY=; b=W69GS 8nnCcNtJ2Ab3NJLQcxBKVIykCVDeZ+bLr+Qn/DU+YoDRmxGaajinWRz7gtidBAZ/ 9/FmP+CvXY1YfMBezkBMYKlzuxhvB7161fcd5iC5sPtsQ/qsbj4wriotwkAstVcR D6wMMm7Hd6QmD5OGkuuR5RlMC2MBFBWpdibnaY= Received: from localhost (unknown [IPv6:2a01:4f8:120:624c::25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: spectre) by aloka.lostca.se (Postfix) with ESMTPSA id B28EA4DD8; Tue, 23 Mar 2021 16:08:51 +0000 (UTC) Date: Tue, 23 Mar 2021 16:08:51 +0000 From: Arjun Shankar To: Adhemerval Zanella Cc: libc-alpha@sourceware.org, Paul Eggert , bug-gnulib@gnu.org Subject: Re: [PATCH 2/8] posix: Use char_array for internal glob dirname Message-ID: <20210323160851.GA6872@aloka.lostca.se> References: <20210105185820.3796657-1-adhemerval.zanella@linaro.org> <20210105185820.3796657-3-adhemerval.zanella@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210105185820.3796657-3-adhemerval.zanella@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Mar 2021 16:09:03 -0000 Hi Adhemerval, > This is the first patch of the set to remove alloca usage on glob > implementation. Internal path to search for file might expand to a > non static directory derived from pattern for some difference cases > (GLOB_NOESCAPE, GNU GLOB_TILDE) and to allow a non-static dirname > path glob uses a lot of boilerplate code to manage the buffer (which > is either allocated using alloca or malloc depending both to size > requested and the total alloca_used). > > The patch changes to use the char_array struct with the default size > (256 bytes). It simplifies all the allocation code by using char_array > one and every internal buffer access is done using char_array provided > functions. No functional changes are expected. I've been going through this patch series. Here are my comments on the first one. I have mostly been looking at this from the point of view of making sure the logic remains equivalent, and not the way you tackled the problem itself. In summary, I found a comparison reversed, a missed concatenation, and some minor indent issues. Other than that, this patch looks good to me. Details: > diff --git a/posix/glob.c b/posix/glob.c > index 32c88e5d15..8c6e1914c5 100644 > --- a/posix/glob.c > +++ b/posix/glob.c > @@ -85,6 +85,7 @@ > #include > #include > #include > +#include Include the new dynamic character array implementation. OK. Note: The patch that introduces char_array-skeleton.c will need a slight adjustment after de0e1b45b0ab (malloc: Sync dynarray with gnulib) due to the removal of the anonymous union. > static const char *next_brace_sub (const char *begin, int flags) __THROWNL; > > @@ -298,16 +299,15 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > glob_t *pglob) > { > const char *filename; > - char *dirname = NULL; > size_t dirlen; > int status; > size_t oldcount; > int meta; > - int dirname_modified; > - int malloc_dirname = 0; > glob_t dirs; > int retval = 0; > size_t alloca_used = 0; > + struct char_array dirname; > + bool dirname_modified; dirname changes type, dirname_modified should be a boolean, and malloc_dirname is now redundant. OK. > if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0) > { > @@ -315,6 +315,10 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > return -1; > } > > + /* Default char array is stack allocated, so there is no need to check > + if setting the initial '\0' succeeds. */ > + char_array_init_empty (&dirname); > + > /* POSIX requires all slashes to be matched. This means that with > a trailing slash we must match only directories. */ > if (pattern[0] && pattern[strlen (pattern) - 1] == '/') OK. > @@ -335,12 +339,12 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > size_t i; > > if (pglob->gl_offs >= ~((size_t) 0) / sizeof (char *)) > - return GLOB_NOSPACE; > + goto err_nospace; > > pglob->gl_pathv = (char **) malloc ((pglob->gl_offs + 1) > * sizeof (char *)); err_nospace frees dirname and returns GLOB_NOSPACE. So the code is equivalent. OK. > if (pglob->gl_pathv == NULL) > - return GLOB_NOSPACE; > + goto err_nospace; > > for (i = 0; i <= pglob->gl_offs; ++i) > pglob->gl_pathv[i] = NULL; > @@ -392,7 +396,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > { > onealt = malloc (pattern_len); > if (onealt == NULL) > - return GLOB_NOSPACE; > + goto err_nospace; > } > > /* We know the prefix for all sub-patterns. */ OK. Same. > @@ -454,7 +458,8 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > globfree (pglob); > pglob->gl_pathc = 0; > } > - return result; > + retval = result; > + goto out; > } > > if (*next == '}') out frees dirname and returns retval. So the code is equivalent. OK. > @@ -471,9 +476,10 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > > if (pglob->gl_pathc != firstc) > /* We found some entries. */ > - return 0; > + retval = 0; We will continue at 'out', which will also return after freeing dirname. So the code remains equivalent. OK. > else if (!(flags & (GLOB_NOCHECK|GLOB_NOMAGIC))) > - return GLOB_NOMATCH; > + retval = GLOB_NOMATCH; > + goto out; > } > } > OK. Same here. > @@ -492,14 +498,15 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > filename = strchr (pattern, ':'); > #endif /* __MSDOS__ || WINDOWS32 */ > > - dirname_modified = 0; > + dirname_modified = false; > if (filename == NULL) > { OK. It's declared as a boolean now. > /* This can mean two things: a simple name or "~name". The latter > case is nothing but a notation for a directory. */ > if ((flags & (GLOB_TILDE|GLOB_TILDE_CHECK)) && pattern[0] == '~') > { > - dirname = (char *) pattern; > + if (!char_array_set_str (&dirname, pattern)) > + goto err_nospace; > dirlen = strlen (pattern); > > /* Set FILENAME to NULL as a special flag. This is ugly but OK. It's still equivalent. Since char_array_set_str can lead to a failed allocation, we add a check and exit with error if that happens. Indent is a bit off, though. > @@ -516,7 +523,8 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > } > > filename = pattern; > - dirname = (char *) "."; > + if (!char_array_set_str (&dirname, ".")) > + goto err_nospace; > dirlen = 0; > } > } OK. Same as last. Return an error if the allocation fails. Indent is a bit off. > @@ -525,13 +533,13 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > && (flags & GLOB_NOESCAPE) == 0)) > { > /* "/pattern" or "\\/pattern". */ > - dirname = (char *) "/"; > + if (!char_array_set_str (&dirname, "/")) > + goto err_nospace; > dirlen = 1; > ++filename; > } OK. > else > { > - char *newp; > dirlen = filename - pattern; > #if defined __MSDOS__ || defined WINDOWS32 > if (*filename == ':' OK. newp was used for malloc/alloca allocations. > @@ -545,31 +553,26 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > /* For now, disallow wildcards in the drive spec, to > prevent infinite recursion in glob. */ > if (__glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE))) > - return GLOB_NOMATCH; > + { > + retval = GLOB_NOMATCH; > + goto out; > + } We need to deallocate before every return now. This does that. OK. > /* If this is "d:pattern", we need to copy ':' to DIRNAME > as well. If it's "d:/pattern", don't remove the slash > from "d:/", since "d:" and "d:/" are not the same.*/ > } > #endif > > - if (glob_use_alloca (alloca_used, dirlen + 1)) > - newp = alloca_account (dirlen + 1, alloca_used); > - else > - { > - newp = malloc (dirlen + 1); > - if (newp == NULL) > - return GLOB_NOSPACE; > - malloc_dirname = 1; > - } > - *((char *) mempcpy (newp, pattern, dirlen)) = '\0'; > - dirname = newp; > + if (!char_array_set_str_size (&dirname, pattern, dirlen)) > + goto err_nospace; > ++filename; We used to alloca/malloc some space and copy pattern into it. We still do the same but using a dynarray. So don't need malloc_dirname any more. OK. > #if defined __MSDOS__ || defined WINDOWS32 > bool drive_root = (dirlen > 1 > - && (dirname[dirlen - 1] == ':' > - || (dirlen > 2 && dirname[dirlen - 2] == ':' > - && dirname[dirlen - 1] == '/'))); > + && (char_array_pos (&dirname, dirlen - 1) != ':' > + || (dirlen > 2 > + && char_array_pos (&dirname, dirlen - 2) != ':' > + && char_array_pos (&dirname, dirlen - 1) != '/'))); > #else > bool drive_root = false; > #endif Looks like the comparisons have been reversed. I think they should be `==' instead of `!='. > @@ -578,20 +581,24 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > /* "pattern/". Expand "pattern", appending slashes. */ > { > int orig_flags = flags; > - if (!(flags & GLOB_NOESCAPE) && dirname[dirlen - 1] == '\\') > + if (!(flags & GLOB_NOESCAPE) > + && char_array_pos (&dirname, dirlen - 1) == '\\') > { OK. > /* "pattern\\/". Remove the final backslash if it hasn't > been quoted. */ > - char *p = (char *) &dirname[dirlen - 1]; > - > - while (p > dirname && p[-1] == '\\') --p; > - if ((&dirname[dirlen] - p) & 1) > + size_t p = dirlen - 1; > + while (p > 0 && char_array_pos (&dirname, p - 1) == '\\') --p; > + if ((dirlen - p) & 1) > { > - *(char *) &dirname[--dirlen] = '\0'; > + /* Since we are shrinking the array, there is no need to > + check the function return. */ > + dirlen -= 1; > + char_array_crop (&dirname, dirlen); > flags &= ~(GLOB_NOCHECK | GLOB_NOMAGIC); > } > } p was a pointer to the last character in dirname, and we looped on it going backwards towards the start of dirname looking for a '\'. Now, p is an index into dirname and we do the same thing. Looks equivalent, and more readable. OK. > - int val = __glob (dirname, flags | GLOB_MARK, errfunc, pglob); > + int val = __glob (char_array_str (&dirname), flags | GLOB_MARK, > + errfunc, pglob); > if (val == 0) > pglob->gl_flags = ((pglob->gl_flags & ~GLOB_MARK) > | (flags & GLOB_MARK)); OK. > @@ -608,11 +615,14 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > } > } > > - if ((flags & (GLOB_TILDE|GLOB_TILDE_CHECK)) && dirname[0] == '~') > + if ((flags & (GLOB_TILDE|GLOB_TILDE_CHECK)) > + && char_array_pos (&dirname, 0) == '~') > { OK. > - if (dirname[1] == '\0' || dirname[1] == '/' > - || (!(flags & GLOB_NOESCAPE) && dirname[1] == '\\' > - && (dirname[2] == '\0' || dirname[2] == '/'))) > + if (char_array_pos (&dirname, 1) == '\0' > + || char_array_pos (&dirname, 1) == '/' > + || (!(flags & GLOB_NOESCAPE) && char_array_pos (&dirname, 1) == '\\' > + && (char_array_pos (&dirname, 2) == '\0' > + || char_array_pos (&dirname, 2) == '/'))) OK. They do the same thing. Indent needs a fix. > { > /* Look up home directory. */ > char *home_dir = getenv ("HOME"); > @@ -663,10 +673,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > if (err != ERANGE) > break; > if (!scratch_buffer_grow (&s)) > - { > - retval = GLOB_NOSPACE; > - goto out; > - } > + goto err_nospace; > } > if (err == 0) > { OK. > @@ -675,10 +682,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > } > scratch_buffer_free (&s); > if (err == 0 && home_dir == NULL) > - { > - retval = GLOB_NOSPACE; > - goto out; > - } > + goto err_nospace; > #endif /* WINDOWS32 */ > } > if (home_dir == NULL || home_dir[0] == '\0') OK. > @@ -697,53 +701,26 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > } > } > /* Now construct the full directory. */ > - if (dirname[1] == '\0') > + if (char_array_pos (&dirname, 1) == '\0') > { OK. > - if (__glibc_unlikely (malloc_dirname)) > - free (dirname); > - OK. We don't use malloc for dirname any more. > - dirname = home_dir; > - dirlen = strlen (dirname); > - malloc_dirname = malloc_home_dir; > + if (!char_array_set_str (&dirname, home_dir)) > + goto err_nospace; > + dirlen = char_array_size (&dirname) - 1; Equivalent, and we don't use malloc_dirname any more so we throw away that assignment. OK. > } > else > { > - char *newp; > - size_t home_len = strlen (home_dir); > - int use_alloca = glob_use_alloca (alloca_used, home_len + dirlen); > - if (use_alloca) > - newp = alloca_account (home_len + dirlen, alloca_used); > - else > - { > - newp = malloc (home_len + dirlen); > - if (newp == NULL) > - { > - if (__glibc_unlikely (malloc_home_dir)) > - free (home_dir); > - retval = GLOB_NOSPACE; > - goto out; > - } > - } This code was allocating enough memory to concatenate home_dir and dirname. > - mempcpy (mempcpy (newp, home_dir, home_len), > - &dirname[1], dirlen); ...Then concatenating it. > - if (__glibc_unlikely (malloc_dirname)) > - free (dirname); > - > - dirname = newp; > - dirlen += home_len - 1; > - malloc_dirname = !use_alloca; > - > - if (__glibc_unlikely (malloc_home_dir)) > - free (home_dir); ...And freeing any previously allocated memory. > + /* Replaces '~' by the obtained HOME dir. */ > + char_array_erase (&dirname, 0); > + if (!char_array_prepend_str (&dirname, home_dir)) > + goto err_nospace; Now we do it using a dynarray. OK. Indent needs a fix. > } > - dirname_modified = 1; > + dirname_modified = true; > } OK. > else > { > #ifndef WINDOWS32 > - char *end_name = strchr (dirname, '/'); > + char *dirnamestr = char_array_at (&dirname, 0); > + char *end_name = strchr (dirnamestr, '/'); Equivalent. OK. > char *user_name; > int malloc_user_name = 0; > char *unescape = NULL; > @@ -752,23 +729,23 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > { > if (end_name == NULL) > { > - unescape = strchr (dirname, '\\'); > + unescape = strchr (dirnamestr, '\\'); > if (unescape) > end_name = strchr (unescape, '\0'); > } OK. Indent needs a fix, and further down as well. > else > - unescape = memchr (dirname, '\\', end_name - dirname); > + unescape = memchr (dirnamestr, '\\', end_name - dirnamestr); > } OK. > if (end_name == NULL) > - user_name = dirname + 1; > + user_name = dirnamestr + 1; OK. > else > { > char *newp; > - if (glob_use_alloca (alloca_used, end_name - dirname)) > - newp = alloca_account (end_name - dirname, alloca_used); > + if (glob_use_alloca (alloca_used, end_name - dirnamestr)) > + newp = alloca_account (end_name - dirnamestr, alloca_used); > else > { > - newp = malloc (end_name - dirname); > + newp = malloc (end_name - dirnamestr); > if (newp == NULL) > { > retval = GLOB_NOSPACE; OK. This newp is for user_name which is tackled in a separate patch. > @@ -778,8 +755,8 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > } > if (unescape != NULL) > { > - char *p = mempcpy (newp, dirname + 1, > - unescape - dirname - 1); > + char *p = mempcpy (newp, dirnamestr + 1, > + unescape - dirnamestr - 1); > char *q = unescape; > while (q != end_name) > { > @@ -801,8 +778,9 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > *p = '\0'; > } > else > - *((char *) mempcpy (newp, dirname + 1, end_name - dirname - 1)) > - = '\0'; > + *((char *) mempcpy (newp, dirnamestr + 1, > + end_name - dirnamestr - 1)) > + = '\0'; > user_name = newp; > } Same. OK. There appears to be a changed line due to a stray whitespace. > @@ -835,39 +813,14 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > /* If we found a home directory use this. */ > if (p != NULL) > { > - size_t home_len = strlen (p->pw_dir); > - size_t rest_len = end_name == NULL ? 0 : strlen (end_name); > - /* dirname contains end_name; we can't free it now. */ > - char *prev_dirname = > - (__glibc_unlikely (malloc_dirname) ? dirname : NULL); > - char *d; > - > - malloc_dirname = 0; > - > - if (glob_use_alloca (alloca_used, home_len + rest_len + 1)) > - dirname = alloca_account (home_len + rest_len + 1, > - alloca_used); > - else > + if (!char_array_set_str (&dirname, p->pw_dir)) > { > - dirname = malloc (home_len + rest_len + 1); > - if (dirname == NULL) > - { > - free (prev_dirname); > - scratch_buffer_free (&pwtmpbuf); > - retval = GLOB_NOSPACE; > - goto out; > - } > - malloc_dirname = 1; > + scratch_buffer_free (&pwtmpbuf); > + goto err_nospace; > } > - d = mempcpy (dirname, p->pw_dir, home_len); > - if (end_name != NULL) > - d = mempcpy (d, end_name, rest_len); > - *d = '\0'; > - > - free (prev_dirname); > > - dirlen = home_len + rest_len; > - dirname_modified = 1; > + dirlen = strlen (p->pw_dir); > + dirname_modified = true; > } It appears that a concatenation (p->pw_dir and end_name) got missed here. > else > { > @@ -908,37 +861,33 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > goto nospace; > pglob->gl_pathv = new_gl_pathv; > > - if (flags & GLOB_MARK && is_dir (dirname, flags, pglob)) > + if (flags & GLOB_MARK > + && is_dir (char_array_str (&dirname), flags, pglob)) OK. > { > char *p; > pglob->gl_pathv[newcount] = malloc (dirlen + 2); > if (pglob->gl_pathv[newcount] == NULL) > goto nospace; > - p = mempcpy (pglob->gl_pathv[newcount], dirname, dirlen); > + p = mempcpy (pglob->gl_pathv[newcount], > + char_array_str (&dirname), dirlen); OK. > p[0] = '/'; > p[1] = '\0'; > - if (__glibc_unlikely (malloc_dirname)) > - free (dirname); > } OK. > else > { > - if (__glibc_unlikely (malloc_dirname)) > - pglob->gl_pathv[newcount] = dirname; > - else > - { > - pglob->gl_pathv[newcount] = strdup (dirname); > - if (pglob->gl_pathv[newcount] == NULL) > - goto nospace; > - } > + pglob->gl_pathv[newcount] = strdup (char_array_str (&dirname)); > + if (pglob->gl_pathv[newcount] == NULL) > + goto nospace; > } OK. > pglob->gl_pathv[++newcount] = NULL; > ++pglob->gl_pathc; > pglob->gl_flags = flags; > > - return 0; > + goto out; > } OK. 'out' so we can deallocate before returning. > > - meta = __glob_pattern_type (dirname, !(flags & GLOB_NOESCAPE)); > + meta = __glob_pattern_type (char_array_str (&dirname), > + !(flags & GLOB_NOESCAPE)); OK. > /* meta is 1 if correct glob pattern containing metacharacters. > If meta has bit (1 << 2) set, it means there was an unterminated > [ which we handle the same, using fnmatch. Broken unterminated > @@ -951,15 +900,15 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > the pattern in each directory found. */ > size_t i; > > - if (!(flags & GLOB_NOESCAPE) && dirlen > 0 && dirname[dirlen - 1] == '\\') > + if (!(flags & GLOB_NOESCAPE) && dirlen > 0 > + && char_array_pos (&dirname, dirlen - 1) == '\\') OK. > { > /* "foo\\/bar". Remove the final backslash from dirname > if it has not been quoted. */ > - char *p = (char *) &dirname[dirlen - 1]; > - > - while (p > dirname && p[-1] == '\\') --p; > - if ((&dirname[dirlen] - p) & 1) > - *(char *) &dirname[--dirlen] = '\0'; > + size_t p = dirlen - 1; > + while (p > 0 && char_array_pos (&dirname, p - 1) == '\\') --p; > + if ((dirlen - p) & 1) > + char_array_crop (&dirname, --dirlen); Equivalent. We use an index instead of a pointer, and step back from the end. OK. > } > > if (__glibc_unlikely ((flags & GLOB_ALTDIRFUNC) != 0)) > @@ -973,7 +922,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > dirs.gl_lstat = pglob->gl_lstat; > } > > - status = __glob (dirname, > + status = __glob (char_array_str (&dirname), > ((flags & (GLOB_ERR | GLOB_NOESCAPE | GLOB_ALTDIRFUNC)) > | GLOB_NOSORT | GLOB_ONLYDIR), > errfunc, &dirs); OK. > @@ -1020,8 +969,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > globfree (&dirs); > globfree (pglob); > pglob->gl_pathc = 0; > - retval = GLOB_NOSPACE; > - goto out; > + goto err_nospace; OK. > } > } > > @@ -1043,8 +991,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > { > nospace2: > globfree (&dirs); > - retval = GLOB_NOSPACE; > - goto out; > + goto err_nospace; > } > > new_gl_pathv = realloc (pglob->gl_pathv, > @@ -1059,8 +1006,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > globfree (&dirs); > globfree (pglob); > pglob->gl_pathc = 0; > - retval = GLOB_NOSPACE; > - goto out; > + goto err_nospace; > } Same. > > ++pglob->gl_pathc; > @@ -1086,7 +1032,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > > if (meta & GLOBPAT_BACKSLASH) > { > - char *p = strchr (dirname, '\\'), *q; > + char *p = strchr (char_array_str (&dirname), '\\'), *q; Okay. > /* We need to unescape the dirname string. It is certainly > allocated by alloca, as otherwise filename would be NULL > or dirname wouldn't contain backslashes. */ > @@ -1103,12 +1049,12 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > ++q; > } > while (*p++ != '\0'); > - dirname_modified = 1; > + dirname_modified = true; > } > if (dirname_modified) > flags &= ~(GLOB_NOCHECK | GLOB_NOMAGIC); > - status = glob_in_dir (filename, dirname, flags, errfunc, pglob, > - alloca_used); > + status = glob_in_dir (filename, char_array_str (&dirname), flags, > + errfunc, pglob, alloca_used); OK. > if (status != 0) > { > if (status == GLOB_NOMATCH && flags != orig_flags > @@ -1126,14 +1072,13 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > if (dirlen > 0) > { > /* Stick the directory on the front of each name. */ > - if (prefix_array (dirname, > + if (prefix_array (char_array_str (&dirname), > &pglob->gl_pathv[old_pathc + pglob->gl_offs], > pglob->gl_pathc - old_pathc)) OK. > { > globfree (pglob); > pglob->gl_pathc = 0; > - retval = GLOB_NOSPACE; > - goto out; > + goto err_nospace; OK. > } > } > } > @@ -1152,8 +1097,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > { > globfree (pglob); > pglob->gl_pathc = 0; > - retval = GLOB_NOSPACE; > - goto out; > + goto err_nospace; Same. > } > strcpy (&new[len - 2], "/"); > pglob->gl_pathv[i] = new; > @@ -1169,10 +1113,13 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), > } > > out: > - if (__glibc_unlikely (malloc_dirname)) > - free (dirname); > > + char_array_free (&dirname); > return retval; > + > + err_nospace: > + char_array_free (&dirname); > + return GLOB_NOSPACE; > } OK. out and err_nospace, both deallocate before returning. Cheers, Arjun