* [PATCH 0/8] posix: glob fixes and refactor
@ 2017-11-21 13:55 Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 6/8] posix: Remove alloca usage on glob user_name Adhemerval Zanella
` (8 more replies)
0 siblings, 9 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
This patchset main target is to remove alloca usage on glob by
using a specialized dynarray for C strings (struct char_array).
It does not change any glob semantics, only internal glob buffer
management. It also does not solve the potential stack overflow
due recursive allocation from wildcard in patterns [1] (although
an option would be go to pure dynamic char_array for this case).
[1] http://lists.gnu.org/archive/html/bug-gnulib/2017-10/msg00056.html
Adhemerval Zanella (8):
malloc: Add specialized dynarray for C strings
posix: Use char_array for internal glob dirname
posix: Remove alloca usage for GLOB_BRACE on glob
posix: Remove alloca usage on glob dirname
posix: Use dynarray for globname in glob
posix: Remove alloca usage on glob user_name
posix: Use char_array for home_dir in glob
posix: Remove all alloca usage in glob
ChangeLog | 30 ++
malloc/Makefile | 4 +-
malloc/Versions | 7 +
malloc/char_array-impl.c | 57 ++++
malloc/char_array-skeleton.c | 279 +++++++++++++++++
malloc/char_array.h | 53 ++++
malloc/dynarray.h | 9 +
malloc/dynarray_overflow_failure.c | 31 ++
malloc/malloc-internal.h | 14 +
malloc/tst-char_array.c | 112 +++++++
posix/glob.c | 621 +++++++++++++++----------------------
11 files changed, 837 insertions(+), 380 deletions(-)
create mode 100644 malloc/char_array-impl.c
create mode 100644 malloc/char_array-skeleton.c
create mode 100644 malloc/char_array.h
create mode 100644 malloc/dynarray_overflow_failure.c
create mode 100644 malloc/tst-char_array.c
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/8] posix: Use dynarray for globname in glob
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
` (4 preceding siblings ...)
2017-11-21 13:55 ` [PATCH 8/8] posix: Remove all alloca usage in glob Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 4/8] posix: Remove alloca usage on glob dirname Adhemerval Zanella
` (2 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
This patch uses dynarray at glob internal glob_in_dir function to manage
the various matched patterns. It simplify and removes all the boilerplate
buffer managements required. It also removes the glob_use_alloca, since
it is not used anymore.
Checked on x86_64-linux-gnu.
* posix/glob.c (glob_in_dir): Use dynarray for globnames.
---
ChangeLog | 2 +
posix/glob.c | 127 +++++++++++++++++------------------------------------------
2 files changed, 39 insertions(+), 90 deletions(-)
diff --git a/posix/glob.c b/posix/glob.c
index c83954d..59baf62 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -1188,6 +1188,21 @@ prefix_array (const char *dirname, char **array, size_t n)
return 0;
}
+struct globnames_result
+{
+ char **names;
+ size_t length;
+};
+
+/* Create a dynamic array for C string representing the glob name found. */
+#define DYNARRAY_STRUCT globnames_array
+#define DYNARRAY_ELEMENT_FREE(ptr) free (*ptr)
+#define DYNARRAY_ELEMENT char *
+#define DYNARRAY_PREFIX globnames_array_
+#define DYNARRAY_FINAL_TYPE struct globnames_result
+#define DYNARRAY_INITIAL_SIZE 64
+#include <malloc/dynarray-skeleton.c>
+
/* Like 'glob', but PATTERN is a final pathname component,
and matches are searched for in DIRECTORY.
The GLOB_NOSORT bit in FLAGS is ignored. No sorting is ever done.
@@ -1198,25 +1213,13 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
glob_t *pglob, size_t alloca_used)
{
void *stream = NULL;
-# define GLOBNAMES_MEMBERS(nnames) \
- struct globnames *next; size_t count; char *name[nnames];
- struct globnames { GLOBNAMES_MEMBERS (FLEXIBLE_ARRAY_MEMBER) };
- struct { GLOBNAMES_MEMBERS (64) } init_names_buf;
- struct globnames *init_names = (struct globnames *) &init_names_buf;
- struct globnames *names = init_names;
- struct globnames *names_alloca = init_names;
+ struct globnames_array globnames;
size_t nfound = 0;
- size_t cur = 0;
int meta;
int save;
int result;
- alloca_used += sizeof init_names_buf;
-
- init_names->next = NULL;
- init_names->count = ((sizeof init_names_buf
- - offsetof (struct globnames, name))
- / sizeof init_names->name[0]);
+ globnames_array_init (&globnames);
meta = __glob_pattern_type (pattern, !(flags & GLOB_NOESCAPE));
if (meta == GLOBPAT_NONE && (flags & (GLOB_NOCHECK|GLOB_NOMAGIC)))
@@ -1293,34 +1296,10 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
if (fnmatch (pattern, d.name, fnm_flags) == 0)
{
- if (cur == names->count)
- {
- struct globnames *newnames;
- size_t count = names->count * 2;
- size_t nameoff = offsetof (struct globnames, name);
- size_t size = FLEXSIZEOF (struct globnames, name,
- count * sizeof (char *));
- if ((SIZE_MAX - nameoff) / 2 / sizeof (char *)
- < names->count)
- goto memory_error;
- if (glob_use_alloca (alloca_used, size))
- newnames = names_alloca
- = alloca_account (size, alloca_used);
- else if ((newnames = malloc (size))
- == NULL)
- goto memory_error;
- newnames->count = count;
- newnames->next = names;
- names = newnames;
- cur = 0;
- }
- names->name[cur] = strdup (d.name);
- if (names->name[cur] == NULL)
- goto memory_error;
- ++cur;
- ++nfound;
- if (SIZE_MAX - pglob->gl_offs <= nfound)
+ globnames_array_add (&globnames, strdup (d.name));
+ if (globnames_array_has_failed (&globnames))
goto memory_error;
+ nfound++;
}
}
}
@@ -1330,10 +1309,13 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
{
size_t len = strlen (pattern);
nfound = 1;
- names->name[cur] = malloc (len + 1);
- if (names->name[cur] == NULL)
+ char *newp = malloc (len + 1);
+ if (newp == NULL)
+ goto memory_error;
+ *((char *) mempcpy (newp, pattern, len)) = '\0';
+ globnames_array_add (&globnames, newp);
+ if (globnames_array_has_failed (&globnames))
goto memory_error;
- *((char *) mempcpy (names->name[cur++], pattern, len)) = '\0';
}
result = GLOB_NOMATCH;
@@ -1354,59 +1336,24 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
if (new_gl_pathv == NULL)
{
memory_error:
- while (1)
- {
- struct globnames *old = names;
- for (size_t i = 0; i < cur; ++i)
- free (names->name[i]);
- names = names->next;
- /* NB: we will not leak memory here if we exit without
- freeing the current block assigned to OLD. At least
- the very first block is always allocated on the stack
- and this is the block assigned to OLD here. */
- if (names == NULL)
- {
- assert (old == init_names);
- break;
- }
- cur = names->count;
- if (old == names_alloca)
- names_alloca = names;
- else
- free (old);
- }
+ globnames_array_free (&globnames);
result = GLOB_NOSPACE;
}
else
{
- while (1)
+ struct globnames_result ret = { .names = 0, .length = -1 };
+ if (!globnames_array_finalize (&globnames, &ret))
+ result = GLOB_NOSPACE;
+ else
{
- struct globnames *old = names;
- for (size_t i = 0; i < cur; ++i)
+ for (size_t i = 0; i < ret.length; ++i)
new_gl_pathv[pglob->gl_offs + pglob->gl_pathc++]
- = names->name[i];
- names = names->next;
- /* NB: we will not leak memory here if we exit without
- freeing the current block assigned to OLD. At least
- the very first block is always allocated on the stack
- and this is the block assigned to OLD here. */
- if (names == NULL)
- {
- assert (old == init_names);
- break;
- }
- cur = names->count;
- if (old == names_alloca)
- names_alloca = names;
- else
- free (old);
+ = ret.names[i];
+ pglob->gl_pathv = new_gl_pathv;
+ pglob->gl_pathv[pglob->gl_offs + pglob->gl_pathc] = NULL;
+ pglob->gl_flags = flags;
}
-
- pglob->gl_pathv = new_gl_pathv;
-
- pglob->gl_pathv[pglob->gl_offs + pglob->gl_pathc] = NULL;
-
- pglob->gl_flags = flags;
+ free (ret.names);
}
}
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 7/8] posix: Use char_array for home_dir in glob
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 6/8] posix: Remove alloca usage on glob user_name Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 14:24 ` Andreas Schwab
2017-11-21 13:55 ` [PATCH 2/8] posix: Use char_array for internal glob dirname Adhemerval Zanella
` (6 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
This patch uses dynarray at glob internal home directory ame
manipulation for GLOB_TILDE. It simplifies it and removes all the
boilerplate buffer managements required.
Checked x86_64-linux-gnu.
* posix/glob.c (__glob): Use char_array for home directory.
Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
ChangeLog | 2 ++
posix/glob.c | 66 ++++++++++++++++++++++++++++++++++++------------------------
2 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/posix/glob.c b/posix/glob.c
index aefdb28..71807d6 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -596,9 +596,14 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
|| char_array_pos (&dirname, 2) == '/')))
{
/* Look up home directory. */
- char *home_dir = getenv ("HOME");
- int malloc_home_dir = 0;
- if (home_dir == NULL || home_dir[0] == '\0')
+ struct char_array home_dir;
+
+ const char *home_env = getenv ("HOME");
+ home_env = home_env == NULL ? "" : home_env;
+ if (!char_array_init_str (&home_dir, home_env))
+ goto err_nospace;
+
+ if (char_array_is_empty (&home_dir))
{
#ifdef WINDOWS32
/* Windows NT defines HOMEDRIVE and HOMEPATH. But give
@@ -608,16 +613,21 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
if (home_drive != NULL && home_path != NULL)
{
- size_t home_drive_len = strlen (home_drive);
- size_t home_path_len = strlen (home_path);
- char *mem = alloca (home_drive_len + home_path_len + 1);
-
- memcpy (mem, home_drive, home_drive_len);
- memcpy (mem + home_drive_len, home_path, home_path_len + 1);
- home_dir = mem;
+ if (!char_array_set_str (&home_dir, home_drive)
+ || !char_array_append_str (&home_dir, home_path))
+ {
+ char_array_free (&home_dir);
+ goto err_nospace;
+ }
}
else
- home_dir = "c:/users/default"; /* poor default */
+ {
+ if (!char_array_set_str (&home_dir, "c:/users/default"))
+ {
+ char_array_free (&home_dir);
+ goto err_nospace;
+ }
+ }
#else
int err;
struct passwd *p;
@@ -645,44 +655,48 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
if (!scratch_buffer_grow (&s))
goto err_nospace;
}
- if (err == 0)
- {
- home_dir = strdup (p->pw_dir);
- malloc_home_dir = 1;
- }
+ bool e = (err == 0 && char_array_set_str (&home_dir, p->pw_dir));
scratch_buffer_free (&s);
- if (err == 0 && home_dir == NULL)
+ if (e == false)
goto err_nospace;
#endif /* WINDOWS32 */
}
- if (home_dir == NULL || home_dir[0] == '\0')
+ if (char_array_is_empty (&home_dir))
{
- if (__glibc_unlikely (malloc_home_dir))
- free (home_dir);
if (flags & GLOB_TILDE_CHECK)
{
+ char_array_free (&home_dir);
retval = GLOB_NOMATCH;
goto out;
}
else
{
- home_dir = (char *) "~"; /* No luck. */
- malloc_home_dir = 0;
+ if (!char_array_set_str (&home_dir, "~"))
+ {
+ char_array_free (&home_dir);
+ goto err_nospace;
+ }
}
}
/* Now construct the full directory. */
+ bool e = true;
if (char_array_pos (&dirname, 1) == '\0')
{
- if (!char_array_set_str (&dirname, home_dir))
- goto err_nospace;
+ e = char_array_set_str (&dirname, char_array_str (&home_dir));
dirlen = char_array_size (&dirname) - 1;
}
else
{
/* Replaces '~' by the obtained HOME dir. */
char_array_erase (&dirname, 0);
- if (!char_array_prepend_str (&dirname, home_dir))
- goto err_nospace;
+ e = char_array_prepend_str (&dirname,
+ char_array_str (&home_dir));
+ }
+ if (e == false)
+ {
+ char_array_free (&dirname);
+ char_array_free (&home_dir);
+ goto err_nospace;
}
dirname_modified = true;
}
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/8] malloc: Add specialized dynarray for C strings
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
` (6 preceding siblings ...)
2017-11-21 13:55 ` [PATCH 4/8] posix: Remove alloca usage on glob dirname Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 14:15 ` Andreas Schwab
2017-11-21 14:19 ` Andreas Schwab
2017-12-12 17:02 ` [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
8 siblings, 2 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
This patch adds an specialized dynarray to manage C strings using the
dynarray internal implementation. It uses some private fields from
dynarray and thus it provided specific files to access and manage
the internal string buffer.
For instance:
struct char_array str;
// str == "testing"
char_array_init_str (&str, "testing");
// c == 's'
char c = char_array_pos (&str, 2);
// str = "testing2"
char_array_set_str (&str, "testing2");
// str = "testig2"
char_array_erase (&str, 5);
// str = "123testig2";
char_array_prepend_str (&str, "123");
// len = 10;
size_t len = char_array_length (&str);
// str = "123testig2456";
char_array_append_str (&str, "456");
// str = "123test789";
char_array_replace_str_pos (&str, 7, "789", 3);
The provided function are not extensive and meant mainly to be use in
subsequent glob implementation cleanup. For internal object consistency
only the function provided by char_array.c should be used, including
internal object manipulation.
To check for possible overflows in internal size manipulation a new
function, check_add_wrapv_size_t, is added on malloc-internal. It basically
return whether the addition of two size_t overflows.
Checked on x86_64-linux-gnu.
* malloc/Makefile (test-internal): Add tst-char_array.
(routines): Add dynarray_overflow_failure and char_array-impl.
* malloc/Versions [GLIBC_PRIVATE] (libc): Add
__libc_dynarray_overflow_failure, __char_array_set_str_size,
__char_array_erase, __char_array_prepend_str_size, and
__char_array_replace_str_pos.
* malloc/char_array-impl.c: New file.
* malloc/char_array-skeleton.c: Likewise.
* malloc/char_array.h: Likewise.
* malloc/tst-char-array.c: Likewise.
* malloc/dynarray_overflow_failure.c: Likewise.
* malloc/malloc-internal.h (check_add_overflow_size_t): New function.
Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
ChangeLog | 15 ++
malloc/Makefile | 4 +-
malloc/Versions | 7 +
malloc/char_array-impl.c | 57 ++++++++
malloc/char_array-skeleton.c | 279 +++++++++++++++++++++++++++++++++++++
malloc/char_array.h | 53 +++++++
malloc/dynarray.h | 9 ++
malloc/dynarray_overflow_failure.c | 31 +++++
malloc/malloc-internal.h | 14 ++
malloc/tst-char_array.c | 112 +++++++++++++++
10 files changed, 580 insertions(+), 1 deletion(-)
create mode 100644 malloc/char_array-impl.c
create mode 100644 malloc/char_array-skeleton.c
create mode 100644 malloc/char_array.h
create mode 100644 malloc/dynarray_overflow_failure.c
create mode 100644 malloc/tst-char_array.c
diff --git a/malloc/Makefile b/malloc/Makefile
index 17936fc..8435e7f 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -49,6 +49,7 @@ tests-internal += \
tst-dynarray \
tst-dynarray-fail \
tst-dynarray-at-fail \
+ tst-char_array
ifneq (no,$(have-tunables))
tests += tst-malloc-usable-tunables
@@ -61,7 +62,7 @@ test-srcs = tst-mtrace
routines = malloc morecore mcheck mtrace obstack reallocarray \
scratch_buffer_grow scratch_buffer_grow_preserve \
scratch_buffer_set_array_size \
- dynarray_at_failure \
+ dynarray_at_failure dynarray_overflow_failure \
dynarray_emplace_enlarge \
dynarray_finalize \
dynarray_resize \
@@ -71,6 +72,7 @@ routines = malloc morecore mcheck mtrace obstack reallocarray \
alloc_buffer_copy_bytes \
alloc_buffer_copy_string \
alloc_buffer_create_failure \
+ char_array-impl
install-lib := libmcheck.a
non-lib.a := libmcheck.a
diff --git a/malloc/Versions b/malloc/Versions
index 2357cff..b21fe59 100644
--- a/malloc/Versions
+++ b/malloc/Versions
@@ -81,6 +81,7 @@ libc {
# dynarray support
__libc_dynarray_at_failure;
+ __libc_dynarray_overflow_failure;
__libc_dynarray_emplace_enlarge;
__libc_dynarray_finalize;
__libc_dynarray_resize;
@@ -92,5 +93,11 @@ libc {
__libc_alloc_buffer_copy_bytes;
__libc_alloc_buffer_copy_string;
__libc_alloc_buffer_create_failure;
+
+ # char_array support
+ __char_array_set_str_size;
+ __char_array_erase;
+ __char_array_prepend_str_size;
+ __char_array_replace_str_pos;
}
}
diff --git a/malloc/char_array-impl.c b/malloc/char_array-impl.c
new file mode 100644
index 0000000..5a56bbc
--- /dev/null
+++ b/malloc/char_array-impl.c
@@ -0,0 +1,57 @@
+/* Specialized dynarray for C strings. Implementation file.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <malloc/char_array.h>
+
+void
+__char_array_set_str_size (struct dynarray_header *header, const char *str,
+ size_t size)
+{
+ *((char *) mempcpy (header->array, str, size)) = '\0';
+ header->used = size + 1;
+}
+libc_hidden_def (__char_array_set_str_size)
+
+void
+__char_array_erase (struct dynarray_header *header, size_t pos)
+{
+ char *ppos = header->array + pos;
+ char *lpos = header->array + header->used;
+ ptrdiff_t size = lpos - ppos;
+ memmove (ppos, ppos + 1, size);
+ header->used--;
+}
+libc_hidden_def (__char_array_erase)
+
+void
+__char_array_prepend_str_size (struct dynarray_header *header,
+ const char *str, size_t size, size_t used)
+{
+ memmove (header->array + size, header->array, used);
+ memcpy (header->array, str, size);
+}
+libc_hidden_def (__char_array_prepend_str_size)
+
+void
+__char_array_replace_str_pos (struct dynarray_header *header, size_t pos,
+ const char *str, size_t len)
+{
+ char *start = header->array + pos;
+ *(char *) mempcpy (start, str, len) = '\0';
+}
+libc_hidden_def (__char_array_replace_str_pos)
diff --git a/malloc/char_array-skeleton.c b/malloc/char_array-skeleton.c
new file mode 100644
index 0000000..00d3a30
--- /dev/null
+++ b/malloc/char_array-skeleton.c
@@ -0,0 +1,279 @@
+/* Specialized dynarray for C strings.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+/* This file provides a dynamic C string with an initial stack allocated
+ buffer. Since it is based on dynarray, it provides dynamic size
+ expansion and heap usage for large strings.
+
+ The following parameters are optional:
+
+ CHAR_ARRAY_INITIAL_SIZE
+ The size of the statically allocated array (default is 256). It will
+ be used to define DYNARRAY_INITIAL_SIZE.
+
+ The following functions are provided:
+
+ bool char_array_init_empty (struct char_array *);
+ bool char_array_init_str (struct char_array *, const char *);
+ bool char_array_init_str_size (struct char_array *, const char *, size_t);
+ bool char_array_is_empty (struct char_array *);
+ const char *char_array_str (struct char_array *);
+ char char_array_pos (struct char_array *, size_t);
+ size_t char_array_length (struct char_array *);
+ bool char_array_set_str (struct char_array *, const char *);
+ bool char_array_set_str_size (struct char_array *, const char *, size_t);
+ void char_array_erase (struct char_array *, size_t);
+ bool char_array_crop (struct char_array *, size_t);
+ bool char_array_prepend_str (struct char_array *, const char *);
+ bool char_array_append_str (struct char_array *, const char *);
+ bool char_array_replace_str_pos (struct char_array *, size_t, const char *,
+ size_t);
+
+ For instance:
+
+ struct char_array str;
+
+ // str == "testing"
+ char_array_init_str (&str, "testing");
+
+ // c == 's'
+ char c = char_array_pos (&str, 2);
+
+ // str = "testing2"
+ char_array_set_str (&str, "testing2");
+
+ // str = "testig2"
+ char_array_erase (&str, 5);
+
+ // str = "123testig2";
+ char_array_prepend_str (&str, "123");
+
+ // len = 10;
+ size_t len = char_array_length (&str);
+
+ // str = "123testig2456";
+ char_array_append_str (&str, "456");
+
+ // str = "123test789";
+ char_array_replace_str_pos (&str, 7, "789", 3);
+ */
+
+#define DYNARRAY_STRUCT char_array
+#define DYNARRAY_ELEMENT char
+#define DYNARRAY_PREFIX char_array_
+#ifndef CHAR_ARRAY_INITIAL_SIZE
+# define CHAR_ARRAY_INITIAL_SIZE 256
+#endif
+#define DYNARRAY_INITIAL_SIZE CHAR_ARRAY_INITIAL_SIZE
+#include <malloc/dynarray-skeleton.c>
+
+#include <malloc/malloc-internal.h>
+#include <malloc/char_array.h>
+
+/* Return a const char for the internal C string handled by 'array'. */
+__attribute__ ((unused, nonnull (1)))
+static const char *
+char_array_str (struct char_array *array)
+{
+ return char_array_begin (array);
+}
+
+/* Return the character at position 'pos' from the char_array 'array'. */
+__attribute__ ((unused, nonnull (1)))
+static char
+char_array_pos (struct char_array *array, size_t pos)
+{
+ return *char_array_at (array, pos);
+}
+
+/* Calculate the length of the string, excluding the terminating null. */
+__attribute__ ((unused, nonnull (1)))
+static size_t
+char_array_length (struct char_array *array)
+{
+ /* Exclude the final '\0'. */
+ return array->dynarray_header.used - 1;
+}
+
+/* Copy up 'size' bytes from string 'str' to char_array 'array'. A final
+ '\0' is appended in the char_array. */
+__attribute__ ((unused, nonnull (1, 2)))
+static bool
+char_array_set_str_size (struct char_array *array, const char *str,
+ size_t size)
+{
+ size_t newsize;
+ if (check_add_overflow_size_t (size, 1, &newsize))
+ __libc_dynarray_overflow_failure (size, 1);
+
+ if (!char_array_resize (array, newsize))
+ return false;
+
+ __char_array_set_str_size (&array->dynarray_abstract, str, size);
+ return true;
+}
+
+/* Copy the contents of string 'str' to char_array 'array', including the
+ final '\0'. */
+__attribute__ ((unused, nonnull (1, 2)))
+static bool
+char_array_set_str (struct char_array *array, const char *str)
+{
+ return char_array_set_str_size (array, str, strlen (str));
+}
+
+/* Initialize the char_array 'array' and sets it to an empty string (""). */
+__attribute__ ((unused, nonnull (1)))
+static bool
+char_array_init_empty (struct char_array *array)
+{
+ char_array_init (array);
+ return char_array_set_str (array, "");
+}
+
+/* Initialize the char_array 'array' and copy the content of string 'str'. */
+__attribute__ ((unused, nonnull (1, 2)))
+static bool
+char_array_init_str (struct char_array *array, const char *str)
+{
+ char_array_init (array);
+ return char_array_set_str (array, str);
+}
+
+/* Initialize the char_array 'array' and copy the content of string 'str'
+ up to 'size' characteres. */
+__attribute__ ((unused, nonnull (1, 2)))
+static bool
+char_array_init_str_size (struct char_array *array, const char *str,
+ size_t size)
+{
+ char_array_init (array);
+ return char_array_set_str_size (array, str, size);
+}
+
+/* Return if the char_array contain any characteres. */
+__attribute__ ((unused, nonnull (1)))
+static bool
+char_array_is_empty (struct char_array *array)
+{
+ return *char_array_begin (array) == '\0';
+}
+
+/* Remove the byte at position 'pos' from char_array 'array'. The contents
+ are moved internally if the position is not at the end of the internal
+ buffer. */
+__attribute__ ((unused, nonnull (1)))
+static bool
+char_array_erase (struct char_array *array, size_t pos)
+{
+ if (pos >= array->dynarray_header.used - 1)
+ return false;
+
+ __char_array_erase (&array->dynarray_abstract, pos);
+ return true;
+}
+
+/* Resize the char_array 'array' to size 'count' maintaining the ending
+ '\0' byte. */
+__attribute__ ((unused, nonnull (1)))
+static bool
+char_array_crop (struct char_array *array, size_t size)
+{
+ if (size >= (array->dynarray_header.used - 1)
+ || !char_array_resize (array, size + 1))
+ return false;
+
+ array->dynarray_header.array[size] = '\0';
+ return true;
+}
+
+/* Prepend the contents of string 'str' to char_array 'array', including the
+ final '\0' byte. */
+__attribute__ ((unused, nonnull (1, 2)))
+static bool
+char_array_prepend_str (struct char_array *array, const char *str)
+{
+ size_t size = strlen (str);
+ /* Resizing the array might change its used elements and we need below
+ to correct copy the elements. */
+ size_t used = array->dynarray_header.used;
+
+ size_t newsize;
+ if (check_add_overflow_size_t (used, size, &newsize))
+ __libc_dynarray_overflow_failure (used, size);
+
+ /* Make room for the string and copy it. */
+ if (!char_array_resize (array, newsize))
+ return false;
+ __char_array_prepend_str_size (&array->dynarray_abstract, str, size, used);
+ return true;
+}
+
+/* Append the contents of string 'str' to char_array 'array, including the
+ final '\0' byte. */
+__attribute__ ((unused, nonnull (1, 2)))
+static bool
+char_array_append_str (struct char_array *array, const char *str)
+{
+ size_t size = strlen (str);
+ /* Resizing the array might change its used elements and it used it below
+ to correct copy the elements. */
+ size_t used = array->dynarray_header.used - 1;
+
+ /* 'used' does account for final '\0', so there is no need to add
+ an extra element to calculate the final required size. */
+ size_t newsize;
+ if (check_add_overflow_size_t (used + 1, size, &newsize))
+ __libc_dynarray_overflow_failure (used + 1, size);
+
+ if (!char_array_resize (array, newsize))
+ return false;
+
+ /* Start to append at '\0' up to string length and add a final '\0'. */
+ *(char*) mempcpy (array->dynarray_header.array + used, str, size) = '\0';
+ return true;
+}
+
+/* Append the character 'c' on char_array 'array'. */
+__attribute__ ((unused, nonnull (1)))
+static bool
+char_array_append_char (struct char_array *array, char c)
+{
+ return char_array_append_str (array, (const char[]) { c, '\0' });
+}
+
+/* Replace the contents starting of position 'pos' of char_array 'array'
+ with the contents of string 'str' up to 'len' bytes. A final '\0'
+ is appended in the string. */
+__attribute__ ((unused, nonnull (1, 3)))
+static bool
+char_array_replace_str_pos (struct char_array *array, size_t pos,
+ const char *str, size_t len)
+{
+ if (pos > array->dynarray_header.used)
+ __libc_dynarray_at_failure (array->dynarray_header.used, pos);
+
+ size_t newsize;
+ if (check_add_overflow_size_t (pos, len, &newsize)
+ || check_add_overflow_size_t (newsize, 1, &newsize)
+ || !char_array_resize (array, newsize))
+ return false;
+
+ __char_array_replace_str_pos (&array->dynarray_abstract, pos, str, len);
+ return true;
+}
diff --git a/malloc/char_array.h b/malloc/char_array.h
new file mode 100644
index 0000000..c696673
--- /dev/null
+++ b/malloc/char_array.h
@@ -0,0 +1,53 @@
+/* Specialized dynarray for C strings. Shared definitions.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _CHAR_ARRAY_H
+#define _CHAR_ARRAY_H
+
+#include <malloc/dynarray.h>
+
+/* Internal funciton. Set the dynarray to the content of the string STR up
+ to SIZE bytes. The dynarray must be resized previously. */
+void __char_array_set_str_size (struct dynarray_header *, const char *str,
+ size_t size);
+
+/* Internal function. Remove the character at position POS from dynarray.
+ The position must be a valid one. */
+void __char_array_erase (struct dynarray_header *, size_t pos);
+
+/* Internal function. Prepend the content of string STR up to SIZE bytes to
+ dynarray by moving USED bytes forward. The dynarray must be resized
+ previously. */
+void __char_array_prepend_str_size (struct dynarray_header *,
+ const char *str, size_t size,
+ size_t used);
+
+/* Internal function. Replace the content of dynarray starting at position
+ POS with the content of string STR up to LEN bytes. The dynarray must
+ be resize previously and STR must contain at least LEN bytes. */
+void __char_array_replace_str_pos (struct dynarray_header *, size_t pos,
+ const char *str, size_t len);
+
+#ifndef _ISOMAC
+libc_hidden_proto (__char_array_set_str_size)
+libc_hidden_proto (__char_array_erase)
+libc_hidden_proto (__char_array_prepend_str_size)
+libc_hidden_proto (__char_array_replace_str_pos)
+#endif
+
+#endif
diff --git a/malloc/dynarray.h b/malloc/dynarray.h
index 5888bcb..bb52b0f 100644
--- a/malloc/dynarray.h
+++ b/malloc/dynarray.h
@@ -168,12 +168,21 @@ bool __libc_dynarray_finalize (struct dynarray_header *list, void *scratch,
void __libc_dynarray_at_failure (size_t size, size_t index)
__attribute__ ((noreturn));
+/* Internal function. TErminate the process after an overflow in
+ new size allocation. SIZE is the current number of elements in
+ dynamic array and INCR is the new elements to add on current
+ size. */
+void __libc_dynarray_overflow_failure (size_t size, size_t incr)
+ __attribute__ ((noreturn));
+
#ifndef _ISOMAC
libc_hidden_proto (__libc_dynarray_emplace_enlarge)
libc_hidden_proto (__libc_dynarray_resize)
libc_hidden_proto (__libc_dynarray_resize_clear)
libc_hidden_proto (__libc_dynarray_finalize)
libc_hidden_proto (__libc_dynarray_at_failure)
+libc_hidden_proto (__libc_dynarray_overflow_failure)
+
#endif
#endif /* _DYNARRAY_H */
diff --git a/malloc/dynarray_overflow_failure.c b/malloc/dynarray_overflow_failure.c
new file mode 100644
index 0000000..14936b0
--- /dev/null
+++ b/malloc/dynarray_overflow_failure.c
@@ -0,0 +1,31 @@
+/* Report an dynamic array size overflow condition.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <dynarray.h>
+#include <stdio.h>
+
+void
+__libc_dynarray_overflow_failure (size_t size, size_t incr)
+{
+ char buf[200];
+ __snprintf (buf, sizeof (buf), "Fatal glibc error: "
+ "new size overflows (old %zu and increment %zu)\n",
+ size, incr);
+ __libc_fatal (buf);
+}
+libc_hidden_def (__libc_dynarray_overflow_failure)
diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
index a9c9c6a..3957ebb 100644
--- a/malloc/malloc-internal.h
+++ b/malloc/malloc-internal.h
@@ -91,4 +91,18 @@ check_mul_overflow_size_t (size_t left, size_t right, size_t *result)
#endif
}
+/* Set *R = A + B. Return true if the answer is mathematically incorrect due
+ to overflow; in this case, *R is the low order bits of the correct
+ answer. */
+static inline bool
+check_add_overflow_size_t (size_t a, size_t b, size_t *r)
+{
+#if 5 <= __GNUC__
+ return __builtin_add_overflow (a, b, r);
+#else
+ *r = a + b;
+ return *r < a;
+#endif
+}
+
#endif /* _MALLOC_INTERNAL_H */
diff --git a/malloc/tst-char_array.c b/malloc/tst-char_array.c
new file mode 100644
index 0000000..a3a99fb
--- /dev/null
+++ b/malloc/tst-char_array.c
@@ -0,0 +1,112 @@
+/* Test for char_array.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <string.h>
+
+#include <malloc/char_array-skeleton.c>
+
+#include <malloc.h>
+#include <mcheck.h>
+#include <stdint.h>
+#include <support/check.h>
+#include <support/support.h>
+
+static int
+do_test (void)
+{
+ mtrace ();
+
+ {
+ struct char_array str;
+ TEST_VERIFY_EXIT (char_array_init_empty (&str) == true);
+ TEST_VERIFY_EXIT (char_array_length (&str) == 0);
+ TEST_VERIFY_EXIT (char_array_is_empty (&str) == true);
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "") == 0);
+ char_array_free (&str);
+ }
+
+ {
+ struct char_array str;
+ TEST_VERIFY_EXIT (char_array_init_str (&str, "testing"));
+ TEST_VERIFY_EXIT (char_array_length (&str) == strlen ("testing"));
+ TEST_VERIFY_EXIT (char_array_pos (&str, 2) == 's');
+ TEST_VERIFY_EXIT (char_array_is_empty (&str) == false);
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "testing") == 0);
+ char_array_free (&str);
+ }
+
+ {
+ struct char_array str;
+ TEST_VERIFY_EXIT (char_array_init_str_size (&str, "testing", 4));
+ TEST_VERIFY_EXIT (char_array_length (&str) == 4);
+ TEST_VERIFY_EXIT (char_array_pos (&str, 2) == 's');
+ TEST_VERIFY_EXIT (char_array_is_empty (&str) == false);
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "test") == 0);
+ char_array_free (&str);
+ }
+
+ {
+ struct char_array str;
+ TEST_VERIFY_EXIT (char_array_init_str (&str, "testing"));
+ TEST_VERIFY_EXIT (char_array_set_str (&str, "abcdef"));
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "abcdef") == 0);
+ TEST_VERIFY_EXIT (char_array_set_str_size (&str, "abcdef", 4));
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "abcd") == 0);
+ char_array_free (&str);
+ }
+
+ {
+ struct char_array str;
+ TEST_VERIFY_EXIT (char_array_init_str (&str, "testing"));
+ TEST_VERIFY_EXIT (char_array_erase (&str, 4) == true);
+ TEST_VERIFY_EXIT (char_array_length (&str) == strlen ("testing") - 1);
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "testng") == 0);
+ TEST_VERIFY_EXIT (char_array_erase (&str, char_array_length (&str))
+ == false);
+ TEST_VERIFY_EXIT (char_array_length (&str) == strlen ("testing") - 1);
+ TEST_VERIFY_EXIT (char_array_erase (&str, char_array_length (&str) - 1)
+ == true);
+ TEST_VERIFY_EXIT (char_array_length (&str) == strlen ("testing") - 2);
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "testn") == 0);
+ char_array_free (&str);
+ }
+
+ {
+ struct char_array str;
+ TEST_VERIFY_EXIT (char_array_init_str (&str, "test"));
+ TEST_VERIFY_EXIT (char_array_prepend_str (&str, "123"));
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "123test") == 0);
+ TEST_VERIFY_EXIT (char_array_length (&str) == strlen ("123test"));
+ TEST_VERIFY_EXIT (char_array_append_str (&str, "456"));
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "123test456") == 0);
+ TEST_VERIFY_EXIT (char_array_length (&str) == strlen ("123test456"));
+ TEST_VERIFY_EXIT (char_array_replace_str_pos (&str, 7, "789", 3));
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "123test789") == 0);
+ TEST_VERIFY_EXIT (char_array_length (&str) == strlen ("123test789"));
+ TEST_VERIFY_EXIT (char_array_crop (&str, 7));
+ TEST_VERIFY_EXIT (char_array_length (&str) == 7);
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "123test") == 0);
+ TEST_VERIFY_EXIT (char_array_append_char (&str, '4'));
+ TEST_VERIFY_EXIT (strcmp (char_array_str (&str), "123test4") == 0);
+ char_array_free (&str);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/8] posix: Use char_array for internal glob dirname
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 6/8] posix: Remove alloca usage on glob user_name Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 7/8] posix: Use char_array for home_dir in glob Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 3/8] posix: Remove alloca usage for GLOB_BRACE on glob Adhemerval Zanella
` (5 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
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.
Checked on x86_64-linux-gnu.
* posix/globc.c (glob): Use char_array for dirname.
Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
ChangeLog | 2 +
posix/glob.c | 270 ++++++++++++++++++++++++-----------------------------------
2 files changed, 112 insertions(+), 160 deletions(-)
diff --git a/posix/glob.c b/posix/glob.c
index cb39779..7a89d2f 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -77,6 +77,7 @@
#include <flexmember.h>
#include <glob_internal.h>
#include <scratch_buffer.h>
+#include <malloc/char_array-skeleton.c>
\f
static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
@@ -288,16 +289,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;
+ bool dirname_modified;
glob_t dirs;
int retval = 0;
size_t alloca_used = 0;
+ struct char_array dirname;
if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
{
@@ -305,6 +305,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] == '/')
@@ -325,12 +329,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 *));
if (pglob->gl_pathv == NULL)
- return GLOB_NOSPACE;
+ goto err_nospace;
for (i = 0; i <= pglob->gl_offs; ++i)
pglob->gl_pathv[i] = NULL;
@@ -382,7 +386,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. */
@@ -444,7 +448,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 == '}')
@@ -461,9 +466,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;
else if (!(flags & (GLOB_NOCHECK|GLOB_NOMAGIC)))
- return GLOB_NOMATCH;
+ retval = GLOB_NOMATCH;
+ goto out;
}
}
@@ -482,14 +488,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)
{
/* 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
@@ -506,7 +513,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;
}
}
@@ -515,13 +523,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;
}
else
{
- char *newp;
dirlen = filename - pattern;
#if defined __MSDOS__ || defined WINDOWS32
if (*filename == ':'
@@ -535,31 +543,25 @@ __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;
+ }
/* 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;
#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
@@ -568,20 +570,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) == '\\')
{
/* "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);
}
}
- 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));
@@ -598,11 +604,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) == '~')
{
- 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) == '/')))
{
/* Look up home directory. */
char *home_dir = getenv ("HOME");
@@ -652,10 +661,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)
{
@@ -664,10 +670,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')
@@ -686,53 +689,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')
{
- if (__glibc_unlikely (malloc_dirname))
- free (dirname);
-
- 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;
}
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;
- }
- }
-
- mempcpy (mempcpy (newp, home_dir, home_len),
- &dirname[1], dirlen);
-
- 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);
+ /* Replaces '~' by the obtained HOME dir. */
+ char_array_erase (&dirname, 0);
+ if (!char_array_prepend_str (&dirname, home_dir))
+ goto err_nospace;
}
- dirname_modified = 1;
+ dirname_modified = true;
}
else
{
#ifndef WINDOWS32
- char *end_name = strchr (dirname, '/');
+ char *dirnamestr = char_array_at (&dirname, 0);
+ char *end_name = strchr (dirnamestr, '/');
char *user_name;
int malloc_user_name = 0;
char *unescape = NULL;
@@ -741,23 +717,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');
}
else
- unescape = memchr (dirname, '\\', end_name - dirname);
+ unescape = memchr (dirnamestr, '\\', end_name - dirnamestr);
}
if (end_name == NULL)
- user_name = dirname + 1;
+ user_name = dirnamestr + 1;
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;
@@ -767,8 +743,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)
{
@@ -790,7 +766,8 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
*p = '\0';
}
else
- *((char *) mempcpy (newp, dirname + 1, end_name - dirname - 1))
+ *((char *) mempcpy (newp, dirnamestr + 1,
+ end_name - dirnamestr - 1))
= '\0';
user_name = newp;
}
@@ -824,32 +801,13 @@ __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);
-
- if (__glibc_unlikely (malloc_dirname))
- free (dirname);
- 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)
- {
- scratch_buffer_free (&pwtmpbuf);
- retval = GLOB_NOSPACE;
- goto out;
- }
- malloc_dirname = 1;
+ scratch_buffer_free (&pwtmpbuf);
+ goto err_nospace;
}
- *((char *) mempcpy (mempcpy (dirname, p->pw_dir, home_len),
- end_name, rest_len)) = '\0';
-
- dirlen = home_len + rest_len;
- dirname_modified = 1;
+ dirlen = strlen (p->pw_dir);
+ dirname_modified = true;
}
else
{
@@ -890,37 +848,32 @@ __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))
{
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);
p[0] = '/';
p[1] = '\0';
- if (__glibc_unlikely (malloc_dirname))
- free (dirname);
}
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;
}
pglob->gl_pathv[++newcount] = NULL;
++pglob->gl_pathc;
pglob->gl_flags = flags;
-
- return 0;
+ goto out;
}
- meta = __glob_pattern_type (dirname, !(flags & GLOB_NOESCAPE));
+ meta = __glob_pattern_type (char_array_str (&dirname),
+ !(flags & GLOB_NOESCAPE));
/* 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
@@ -933,15 +886,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) == '\\')
{
/* "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);
}
if (__glibc_unlikely ((flags & GLOB_ALTDIRFUNC) != 0))
@@ -955,7 +908,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);
@@ -1002,8 +955,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;
}
}
@@ -1025,8 +977,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,
@@ -1041,8 +992,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;
}
++pglob->gl_pathc;
@@ -1068,7 +1018,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;
/* 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. */
@@ -1085,12 +1035,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);
if (status != 0)
{
if (status == GLOB_NOMATCH && flags != orig_flags
@@ -1108,14 +1058,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))
{
globfree (pglob);
pglob->gl_pathc = 0;
- retval = GLOB_NOSPACE;
- goto out;
+ goto err_nospace;
}
}
}
@@ -1134,8 +1083,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;
}
strcpy (&new[len - 2], "/");
pglob->gl_pathv[i] = new;
@@ -1151,10 +1099,12 @@ __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;
}
#if defined _LIBC && !defined __glob
versioned_symbol (libc, __glob, glob, GLIBC_2_27);
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/8] posix: Remove alloca usage for GLOB_BRACE on glob
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
` (2 preceding siblings ...)
2017-11-21 13:55 ` [PATCH 2/8] posix: Use char_array for internal glob dirname Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 8/8] posix: Remove all alloca usage in glob Adhemerval Zanella
` (4 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
GNU GLOB_BRACE internal implementation constructs a new expression and
calls glob recursively. It then requires a possible large temporary
buffer place the new pattern.
This patch removes the alloca/malloc usage and replaces it with
char_array.
Checked on x86_64-linux-gnu.
* posix/glob.c (glob): Remove alloca usage for onealt.
Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
ChangeLog | 2 ++
posix/glob.c | 53 +++++++++++++++++++++++++++++------------------------
2 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/posix/glob.c b/posix/glob.c
index 7a89d2f..7c0df0b 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -372,25 +372,23 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
/* Allocate working buffer large enough for our work. Note that
we have at least an opening and closing brace. */
size_t firstc;
- char *alt_start;
const char *p;
const char *next;
const char *rest;
size_t rest_len;
- char *onealt;
- size_t pattern_len = strlen (pattern) - 1;
- int alloca_onealt = glob_use_alloca (alloca_used, pattern_len);
- if (alloca_onealt)
- onealt = alloca_account (pattern_len, alloca_used);
- else
- {
- onealt = malloc (pattern_len);
- if (onealt == NULL)
- goto err_nospace;
- }
+ struct char_array onealt;
/* We know the prefix for all sub-patterns. */
- alt_start = mempcpy (onealt, pattern, begin - pattern);
+ ptrdiff_t onealtlen = begin - pattern;
+ if (!char_array_init_str_size (&onealt, pattern, onealtlen))
+ {
+ if (!(flags & GLOB_APPEND))
+ {
+ pglob->gl_pathc = 0;
+ pglob->gl_pathv = NULL;
+ }
+ goto err_nospace;
+ }
/* Find the first sub-pattern and at the same time find the
rest after the closing brace. */
@@ -398,9 +396,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
if (next == NULL)
{
/* It is an invalid expression. */
- illegal_brace:
- if (__glibc_unlikely (!alloca_onealt))
- free (onealt);
+ char_array_free (&onealt);
flags &= ~GLOB_BRACE;
goto no_brace;
}
@@ -411,8 +407,12 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
{
rest = next_brace_sub (rest + 1, flags);
if (rest == NULL)
- /* It is an illegal expression. */
- goto illegal_brace;
+ {
+ /* It is an illegal expression. */
+ char_array_free (&onealt);
+ flags &= ~GLOB_BRACE;
+ goto no_brace;
+ }
}
/* Please note that we now can be sure the brace expression
is well-formed. */
@@ -431,9 +431,16 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
int result;
/* Construct the new glob expression. */
- mempcpy (mempcpy (alt_start, p, next - p), rest, rest_len);
+ ptrdiff_t nextlen = next - p;
+ if (!char_array_replace_str_pos (&onealt, onealtlen, p, nextlen)
+ || !char_array_replace_str_pos (&onealt, onealtlen + nextlen,
+ rest, rest_len))
+ {
+ char_array_free (&onealt);
+ goto err_nospace;
+ }
- result = __glob (onealt,
+ result = __glob (char_array_str (&onealt),
((flags & ~(GLOB_NOCHECK | GLOB_NOMAGIC))
| GLOB_APPEND),
errfunc, pglob);
@@ -441,8 +448,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
/* If we got an error, return it. */
if (result && result != GLOB_NOMATCH)
{
- if (__glibc_unlikely (!alloca_onealt))
- free (onealt);
+ char_array_free (&onealt);
if (!(flags & GLOB_APPEND))
{
globfree (pglob);
@@ -461,8 +467,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
assert (next != NULL);
}
- if (__glibc_unlikely (!alloca_onealt))
- free (onealt);
+ char_array_free (&onealt);
if (pglob->gl_pathc != firstc)
/* We found some entries. */
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 8/8] posix: Remove all alloca usage in glob
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
` (3 preceding siblings ...)
2017-11-21 13:55 ` [PATCH 3/8] posix: Remove alloca usage for GLOB_BRACE on glob Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 5/8] posix: Use dynarray for globname " Adhemerval Zanella
` (3 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
With alloca usage removal from glob this patch wraps it up by removing
all the alloca defines and macros usage.
Checked on x86_64-linux-gnu.
* posix/glob.c (glob_in_dir, glob): Remove alloca_used argument.
Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
ChangeLog | 2 ++
posix/glob.c | 25 ++++++++-----------------
2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/posix/glob.c b/posix/glob.c
index 71807d6..4d40c4c 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -32,7 +32,6 @@
#ifndef WINDOWS32
# include <pwd.h>
-# include <alloca.h>
#endif
#include <errno.h>
@@ -65,9 +64,6 @@
# define __stat64(fname, buf) stat (fname, buf)
# define __fxstatat64(_, d, f, st, flag) fstatat (d, f, st, flag)
# define struct_stat64 struct stat
-# ifndef __MVS__
-# define __alloca alloca
-# endif
# define __readdir readdir
# define COMPILE_GLOB64
#endif /* _LIBC */
@@ -174,12 +170,6 @@ convert_dirent64 (const struct dirent64 *source)
# ifdef GNULIB_defined_closedir
# undef closedir
# endif
-
-/* Just use malloc. */
-# define __libc_use_alloca(n) false
-# define alloca_account(len, avar) ((void) (len), (void) (avar), (void *) 0)
-# define extend_alloca_account(buf, len, newlen, avar) \
- ((void) (buf), (void) (len), (void) (newlen), (void) (avar), (void *) 0)
#endif
static int
@@ -207,7 +197,7 @@ glob_lstat (glob_t *pglob, int flags, const char *fullname)
static int glob_in_dir (const char *pattern, const char *directory,
int flags, int (*errfunc) (const char *, int),
- glob_t *pglob, size_t alloca_used);
+ glob_t *pglob);
static int prefix_array (const char *prefix, char **array, size_t n) __THROWNL;
static int collated_compare (const void *, const void *) __THROWNL;
@@ -273,7 +263,6 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
bool dirname_modified;
glob_t dirs;
int retval = 0;
- size_t alloca_used = 0;
struct char_array dirname;
if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
@@ -520,11 +509,13 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
char *drive_spec;
++dirlen;
- drive_spec = __alloca (dirlen + 1);
+ drive_spec = malloc (dirlen + 1);
*((char *) mempcpy (drive_spec, pattern, dirlen)) = '\0';
/* For now, disallow wildcards in the drive spec, to
prevent infinite recursion in glob. */
- if (__glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE)))
+ int r = __glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE));
+ free (drive_spec);
+ if (r != 0)
{
retval = GLOB_NOMATCH;
goto out;
@@ -920,7 +911,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
status = glob_in_dir (filename, dirs.gl_pathv[i],
((flags | GLOB_APPEND)
& ~(GLOB_NOCHECK | GLOB_NOMAGIC)),
- errfunc, pglob, alloca_used);
+ errfunc, pglob);
if (status == GLOB_NOMATCH)
/* No matches in this directory. Try the next. */
continue;
@@ -1025,7 +1016,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
if (dirname_modified)
flags &= ~(GLOB_NOCHECK | GLOB_NOMAGIC);
status = glob_in_dir (filename, char_array_str (&dirname), flags,
- errfunc, pglob, alloca_used);
+ errfunc, pglob);
if (status != 0)
{
if (status == GLOB_NOMATCH && flags != orig_flags
@@ -1190,7 +1181,7 @@ struct globnames_result
static int
glob_in_dir (const char *pattern, const char *directory, int flags,
int (*errfunc) (const char *, int),
- glob_t *pglob, size_t alloca_used)
+ glob_t *pglob)
{
void *stream = NULL;
struct globnames_array globnames;
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/8] posix: Remove alloca usage on glob user_name
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 7/8] posix: Use char_array for home_dir in glob Adhemerval Zanella
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
This patch uses dynarray at glob internal user name manipulation for
GLOB_TILDE. It simplifies it and removes all the boilerplate buffer
managements required. It also removes the glob_use_alloca, since
it is not used anymore.
Checked on x86_64-linux-gnu.
* posix/glob.c (size_add_wrapv, glob_use_alloca): Remove.
(__glob): Use char_array for user name.
Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
ChangeLog | 3 +++
posix/glob.c | 86 ++++++++++++++++++------------------------------------------
2 files changed, 29 insertions(+), 60 deletions(-)
diff --git a/posix/glob.c b/posix/glob.c
index 59baf62..aefdb28 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -32,13 +32,13 @@
#ifndef WINDOWS32
# include <pwd.h>
+# include <alloca.h>
#endif
#include <errno.h>
#include <dirent.h>
#include <stdlib.h>
#include <string.h>
-#include <alloca.h>
#ifdef _LIBC
# undef strdup
@@ -205,29 +205,6 @@ glob_lstat (glob_t *pglob, int flags, const char *fullname)
: LSTAT64 (fullname, &ust.st64));
}
-/* Set *R = A + B. Return true if the answer is mathematically
- incorrect due to overflow; in this case, *R is the low order
- bits of the correct answer. */
-
-static bool
-size_add_wrapv (size_t a, size_t b, size_t *r)
-{
-#if 5 <= __GNUC__ && !defined __ICC
- return __builtin_add_overflow (a, b, r);
-#else
- *r = a + b;
- return *r < a;
-#endif
-}
-
-static bool
-glob_use_alloca (size_t alloca_used, size_t len)
-{
- size_t size;
- return (!size_add_wrapv (alloca_used, len, &size)
- && __libc_use_alloca (size));
-}
-
static int glob_in_dir (const char *pattern, const char *directory,
int flags, int (*errfunc) (const char *, int),
glob_t *pglob, size_t alloca_used);
@@ -712,11 +689,10 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
else
{
#ifndef WINDOWS32
- char *dirnamestr = char_array_at (&dirname, 0);
- char *end_name = strchr (dirnamestr, '/');
- char *user_name;
- int malloc_user_name = 0;
- char *unescape = NULL;
+ const char *dirnamestr = char_array_str (&dirname);
+ const char *end_name = strchr (dirnamestr, '/');
+ struct char_array user_name;
+ const char *unescape = NULL;
if (!(flags & GLOB_NOESCAPE))
{
@@ -730,27 +706,19 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
unescape = memchr (dirnamestr, '\\', end_name - dirnamestr);
}
if (end_name == NULL)
- user_name = dirnamestr + 1;
+ {
+ if (!char_array_init_str (&user_name, dirnamestr + 1))
+ goto err_nospace;
+ }
else
{
- char *newp;
- if (glob_use_alloca (alloca_used, end_name - dirnamestr))
- newp = alloca_account (end_name - dirnamestr, alloca_used);
- else
- {
- newp = malloc (end_name - dirnamestr);
- if (newp == NULL)
- {
- retval = GLOB_NOSPACE;
- goto out;
- }
- malloc_user_name = 1;
- }
if (unescape != NULL)
{
- char *p = mempcpy (newp, dirnamestr + 1,
- unescape - dirnamestr - 1);
- char *q = unescape;
+ ptrdiff_t name_len = unescape - dirnamestr - 1;
+ if (!char_array_init_str_size (&user_name, dirnamestr + 1,
+ name_len))
+ goto err_nospace;
+ const char *q = unescape;
while (q != end_name)
{
if (*q == '\\')
@@ -761,20 +729,21 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
but "~fo\\o\\/" unescape to user_name
"foo". */
if (filename == NULL)
- *p++ = '\\';
+ char_array_append_char (&user_name, '\\');
break;
}
++q;
}
- *p++ = *q++;
+ char_array_append_char (&user_name, *q++);
}
- *p = '\0';
}
else
- *((char *) mempcpy (newp, dirnamestr + 1,
- end_name - dirnamestr - 1))
- = '\0';
- user_name = newp;
+ {
+ ptrdiff_t name_len = end_name - dirnamestr - 1;
+ if (!char_array_init_str_size (&user_name, dirnamestr + 1,
+ name_len))
+ goto err_nospace;
+ }
}
/* Look up specific user's home directory. */
@@ -786,7 +755,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
# if defined HAVE_GETPWNAM_R || defined _LIBC
struct passwd pwbuf;
- while (getpwnam_r (user_name, &pwbuf,
+ while (getpwnam_r (char_array_str (&user_name), &pwbuf,
pwtmpbuf.data, pwtmpbuf.length, &p)
== ERANGE)
{
@@ -797,11 +766,10 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
}
}
# else
- p = getpwnam (user_name);
+ p = getpwnam (char_array_str (&user_name));
# endif
- if (__glibc_unlikely (malloc_user_name))
- free (user_name);
+ char_array_free (&user_name);
/* If we found a home directory use this. */
if (p != NULL)
@@ -1024,9 +992,7 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
if (meta & GLOBPAT_BACKSLASH)
{
char *p = strchr (char_array_str (&dirname), '\\'), *q;
- /* 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. */
+ /* We need to unescape the dirname string. */
q = p;
do
{
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/8] posix: Remove alloca usage on glob dirname
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
` (5 preceding siblings ...)
2017-11-21 13:55 ` [PATCH 5/8] posix: Use dynarray for globname " Adhemerval Zanella
@ 2017-11-21 13:55 ` Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 1/8] malloc: Add specialized dynarray for C strings Adhemerval Zanella
2017-12-12 17:02 ` [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
8 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 13:55 UTC (permalink / raw)
To: libc-alpha
This patch replaces the alloca/malloc usage for dirname creation
by the char_array struct.
Checked on x86_64-linux-gnu.
* posix/glob.c (glob_in_dir): Remove alloca usage for fullname.
Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
ChangeLog | 2 ++
posix/glob.c | 28 +++++++++-------------------
2 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/posix/glob.c b/posix/glob.c
index 7c0df0b..c83954d 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -1197,7 +1197,6 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
int (*errfunc) (const char *, int),
glob_t *pglob, size_t alloca_used)
{
- size_t dirlen = strlen (directory);
void *stream = NULL;
# define GLOBNAMES_MEMBERS(nnames) \
struct globnames *next; size_t count; char *name[nnames];
@@ -1229,32 +1228,23 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
}
else if (meta == GLOBPAT_NONE)
{
- size_t patlen = strlen (pattern);
- size_t fullsize;
- bool alloca_fullname
- = (! size_add_wrapv (dirlen + 1, patlen + 1, &fullsize)
- && glob_use_alloca (alloca_used, fullsize));
- char *fullname;
- if (alloca_fullname)
- fullname = alloca_account (fullsize, alloca_used);
- else
+ struct char_array fullname;
+
+ if (!char_array_init_str (&fullname, directory)
+ || !char_array_append_str (&fullname, "/")
+ || !char_array_append_str (&fullname, pattern))
{
- fullname = malloc (fullsize);
- if (fullname == NULL)
- return GLOB_NOSPACE;
+ char_array_free (&fullname);
+ return GLOB_NOSPACE;
}
- mempcpy (mempcpy (mempcpy (fullname, directory, dirlen),
- "/", 1),
- pattern, patlen + 1);
- if (glob_lstat (pglob, flags, fullname) == 0
+ if (glob_lstat (pglob, flags, char_array_str (&fullname)) == 0
|| errno == EOVERFLOW)
/* We found this file to be existing. Now tell the rest
of the function to copy this name into the result. */
flags |= GLOB_NOCHECK;
- if (__glibc_unlikely (!alloca_fullname))
- free (fullname);
+ char_array_free (&fullname);
}
else
{
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/8] malloc: Add specialized dynarray for C strings
2017-11-21 13:55 ` [PATCH 1/8] malloc: Add specialized dynarray for C strings Adhemerval Zanella
@ 2017-11-21 14:15 ` Andreas Schwab
2017-11-21 16:20 ` Adhemerval Zanella
2017-11-21 14:19 ` Andreas Schwab
1 sibling, 1 reply; 18+ messages in thread
From: Andreas Schwab @ 2017-11-21 14:15 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Nov 21 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> diff --git a/malloc/dynarray.h b/malloc/dynarray.h
> index 5888bcb..bb52b0f 100644
> --- a/malloc/dynarray.h
> +++ b/malloc/dynarray.h
> @@ -168,12 +168,21 @@ bool __libc_dynarray_finalize (struct dynarray_header *list, void *scratch,
> void __libc_dynarray_at_failure (size_t size, size_t index)
> __attribute__ ((noreturn));
>
> +/* Internal function. TErminate the process after an overflow in
Terminate
> diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
> index a9c9c6a..3957ebb 100644
> --- a/malloc/malloc-internal.h
> +++ b/malloc/malloc-internal.h
> @@ -91,4 +91,18 @@ check_mul_overflow_size_t (size_t left, size_t right, size_t *result)
> #endif
> }
>
> +/* Set *R = A + B. Return true if the answer is mathematically incorrect due
> + to overflow; in this case, *R is the low order bits of the correct
> + answer. */
> +static inline bool
> +check_add_overflow_size_t (size_t a, size_t b, size_t *r)
> +{
> +#if 5 <= __GNUC__
__GNUC__ >= 5
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/8] malloc: Add specialized dynarray for C strings
2017-11-21 13:55 ` [PATCH 1/8] malloc: Add specialized dynarray for C strings Adhemerval Zanella
2017-11-21 14:15 ` Andreas Schwab
@ 2017-11-21 14:19 ` Andreas Schwab
2017-11-21 16:32 ` Adhemerval Zanella
1 sibling, 1 reply; 18+ messages in thread
From: Andreas Schwab @ 2017-11-21 14:19 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Nov 21 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> // c == 's'
> char c = char_array_pos (&str, 2);
I'd call it char_array_at.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/8] posix: Use char_array for home_dir in glob
2017-11-21 13:55 ` [PATCH 7/8] posix: Use char_array for home_dir in glob Adhemerval Zanella
@ 2017-11-21 14:24 ` Andreas Schwab
0 siblings, 0 replies; 18+ messages in thread
From: Andreas Schwab @ 2017-11-21 14:24 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Nov 21 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> @@ -645,44 +655,48 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
> if (!scratch_buffer_grow (&s))
> goto err_nospace;
> }
> - if (err == 0)
> - {
> - home_dir = strdup (p->pw_dir);
> - malloc_home_dir = 1;
> - }
> + bool e = (err == 0 && char_array_set_str (&home_dir, p->pw_dir));
> scratch_buffer_free (&s);
> - if (err == 0 && home_dir == NULL)
> + if (e == false)
!e
> /* Now construct the full directory. */
> + bool e = true;
> if (char_array_pos (&dirname, 1) == '\0')
> {
> - if (!char_array_set_str (&dirname, home_dir))
> - goto err_nospace;
> + e = char_array_set_str (&dirname, char_array_str (&home_dir));
> dirlen = char_array_size (&dirname) - 1;
> }
> else
> {
> /* Replaces '~' by the obtained HOME dir. */
> char_array_erase (&dirname, 0);
> - if (!char_array_prepend_str (&dirname, home_dir))
> - goto err_nospace;
> + e = char_array_prepend_str (&dirname,
> + char_array_str (&home_dir));
> + }
> + if (e == false)
!e
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/8] malloc: Add specialized dynarray for C strings
2017-11-21 14:15 ` Andreas Schwab
@ 2017-11-21 16:20 ` Adhemerval Zanella
0 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 16:20 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
On 21/11/2017 12:15, Andreas Schwab wrote:
> On Nov 21 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> diff --git a/malloc/dynarray.h b/malloc/dynarray.h
>> index 5888bcb..bb52b0f 100644
>> --- a/malloc/dynarray.h
>> +++ b/malloc/dynarray.h
>> @@ -168,12 +168,21 @@ bool __libc_dynarray_finalize (struct dynarray_header *list, void *scratch,
>> void __libc_dynarray_at_failure (size_t size, size_t index)
>> __attribute__ ((noreturn));
>>
>> +/* Internal function. TErminate the process after an overflow in
>
> Terminate
>
>> diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
>> index a9c9c6a..3957ebb 100644
>> --- a/malloc/malloc-internal.h
>> +++ b/malloc/malloc-internal.h
>> @@ -91,4 +91,18 @@ check_mul_overflow_size_t (size_t left, size_t right, size_t *result)
>> #endif
>> }
>>
>> +/* Set *R = A + B. Return true if the answer is mathematically incorrect due
>> + to overflow; in this case, *R is the low order bits of the correct
>> + answer. */
>> +static inline bool
>> +check_add_overflow_size_t (size_t a, size_t b, size_t *r)
>> +{
>> +#if 5 <= __GNUC__
>
> __GNUC__ >= 5
>
> Andreas.
>
I fixed both locally, thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/8] malloc: Add specialized dynarray for C strings
2017-11-21 14:19 ` Andreas Schwab
@ 2017-11-21 16:32 ` Adhemerval Zanella
0 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-11-21 16:32 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
On 21/11/2017 12:19, Andreas Schwab wrote:
> On Nov 21 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> // c == 's'
>> char c = char_array_pos (&str, 2);
>
> I'd call it char_array_at.
>
> Andreas.
>
Since char_array is based directly on dynarray the name is already implemented
by it. In fact we do have a char_array_at, but it returns a 'char *' to the
position.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/8] posix: glob fixes and refactor
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
` (7 preceding siblings ...)
2017-11-21 13:55 ` [PATCH 1/8] malloc: Add specialized dynarray for C strings Adhemerval Zanella
@ 2017-12-12 17:02 ` Adhemerval Zanella
8 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2017-12-12 17:02 UTC (permalink / raw)
To: libc-alpha
Ping.
On 21/11/2017 11:55, Adhemerval Zanella wrote:
> This patchset main target is to remove alloca usage on glob by
> using a specialized dynarray for C strings (struct char_array).
>
> It does not change any glob semantics, only internal glob buffer
> management. It also does not solve the potential stack overflow
> due recursive allocation from wildcard in patterns [1] (although
> an option would be go to pure dynamic char_array for this case).
>
> [1] http://lists.gnu.org/archive/html/bug-gnulib/2017-10/msg00056.html
>
> Adhemerval Zanella (8):
> malloc: Add specialized dynarray for C strings
> posix: Use char_array for internal glob dirname
> posix: Remove alloca usage for GLOB_BRACE on glob
> posix: Remove alloca usage on glob dirname
> posix: Use dynarray for globname in glob
> posix: Remove alloca usage on glob user_name
> posix: Use char_array for home_dir in glob
> posix: Remove all alloca usage in glob
>
> ChangeLog | 30 ++
> malloc/Makefile | 4 +-
> malloc/Versions | 7 +
> malloc/char_array-impl.c | 57 ++++
> malloc/char_array-skeleton.c | 279 +++++++++++++++++
> malloc/char_array.h | 53 ++++
> malloc/dynarray.h | 9 +
> malloc/dynarray_overflow_failure.c | 31 ++
> malloc/malloc-internal.h | 14 +
> malloc/tst-char_array.c | 112 +++++++
> posix/glob.c | 621 +++++++++++++++----------------------
> 11 files changed, 837 insertions(+), 380 deletions(-)
> create mode 100644 malloc/char_array-impl.c
> create mode 100644 malloc/char_array-skeleton.c
> create mode 100644 malloc/char_array.h
> create mode 100644 malloc/dynarray_overflow_failure.c
> create mode 100644 malloc/tst-char_array.c
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/8] posix: Use char_array for internal glob dirname
2021-03-23 16:08 ` Arjun Shankar
@ 2021-03-24 17:39 ` Adhemerval Zanella
0 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2021-03-24 17:39 UTC (permalink / raw)
To: Arjun Shankar; +Cc: libc-alpha, Paul Eggert, bug-gnulib
On 23/03/2021 13:08, Arjun Shankar wrote:
> 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.
Thanks for the review, however I would like to check if this approach is
what he want to move the glob implementation since it might diverge from
what Paul suggested for gnulib [1].
I tried to model the change to use something similar to a C++ string,
while Paul is caving out the required allocation from a scratch_buffer.
My view is since we expanding multiple different objects (the directory
name, the home directory, the username) it would make sense to make
then different dynarray, Paul in the other hand see that we can cave out
this multiple objects from a single scratch_buffer to simplify the
memory management. Maybe modelling like a C++ string is not the right
approach, since we need manual delocation anyway.
I don't have a strong opinion here, I will take a second look on his
patch to check if the outcome is simpler than my initial patchset.
My only goal here is get rid of the multuiple alloca and simplify
the memory management.
[1] https://sourceware.org/pipermail/libc-alpha/2021-January/121605.html
>
> 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 <flexmember.h>
>> #include <glob_internal.h>
>> #include <scratch_buffer.h>
>> +#include <malloc/char_array-skeleton.c>
>
> 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.
Ack.
>
>> 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.
Ack.
>
>> @@ -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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/8] posix: Use char_array for internal glob dirname
2021-01-05 18:58 ` [PATCH 2/8] posix: Use char_array for internal glob dirname Adhemerval Zanella
@ 2021-03-23 16:08 ` Arjun Shankar
2021-03-24 17:39 ` Adhemerval Zanella
0 siblings, 1 reply; 18+ messages in thread
From: Arjun Shankar @ 2021-03-23 16:08 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Paul Eggert, bug-gnulib
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 <flexmember.h>
> #include <glob_internal.h>
> #include <scratch_buffer.h>
> +#include <malloc/char_array-skeleton.c>
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/8] posix: Use char_array for internal glob dirname
2021-01-05 18:58 [PATCH 0/8] Remove alloca usage from glob Adhemerval Zanella
@ 2021-01-05 18:58 ` Adhemerval Zanella
2021-03-23 16:08 ` Arjun Shankar
0 siblings, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2021-01-05 18:58 UTC (permalink / raw)
To: libc-alpha, Paul Eggert, bug-gnulib
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.
Checked on x86_64-linux-gnu.
---
posix/glob.c | 275 +++++++++++++++++++++------------------------------
1 file changed, 111 insertions(+), 164 deletions(-)
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 <flexmember.h>
#include <glob_internal.h>
#include <scratch_buffer.h>
+#include <malloc/char_array-skeleton.c>
\f
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;
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] == '/')
@@ -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 *));
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. */
@@ -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 == '}')
@@ -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;
else if (!(flags & (GLOB_NOCHECK|GLOB_NOMAGIC)))
- return GLOB_NOMATCH;
+ retval = GLOB_NOMATCH;
+ goto out;
}
}
@@ -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)
{
/* 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
@@ -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;
}
}
@@ -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;
}
else
{
- char *newp;
dirlen = filename - pattern;
#if defined __MSDOS__ || defined WINDOWS32
if (*filename == ':'
@@ -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;
+ }
/* 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;
#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
@@ -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) == '\\')
{
/* "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);
}
}
- 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));
@@ -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) == '~')
{
- 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) == '/')))
{
/* 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)
{
@@ -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')
@@ -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')
{
- if (__glibc_unlikely (malloc_dirname))
- free (dirname);
-
- 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;
}
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;
- }
- }
-
- mempcpy (mempcpy (newp, home_dir, home_len),
- &dirname[1], dirlen);
-
- 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);
+ /* Replaces '~' by the obtained HOME dir. */
+ char_array_erase (&dirname, 0);
+ if (!char_array_prepend_str (&dirname, home_dir))
+ goto err_nospace;
}
- dirname_modified = 1;
+ dirname_modified = true;
}
else
{
#ifndef WINDOWS32
- char *end_name = strchr (dirname, '/');
+ char *dirnamestr = char_array_at (&dirname, 0);
+ char *end_name = strchr (dirnamestr, '/');
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');
}
else
- unescape = memchr (dirname, '\\', end_name - dirname);
+ unescape = memchr (dirnamestr, '\\', end_name - dirnamestr);
}
if (end_name == NULL)
- user_name = dirname + 1;
+ user_name = dirnamestr + 1;
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;
@@ -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;
}
@@ -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;
}
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))
{
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);
p[0] = '/';
p[1] = '\0';
- if (__glibc_unlikely (malloc_dirname))
- free (dirname);
}
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;
}
pglob->gl_pathv[++newcount] = NULL;
++pglob->gl_pathc;
pglob->gl_flags = flags;
- return 0;
+ goto out;
}
- meta = __glob_pattern_type (dirname, !(flags & GLOB_NOESCAPE));
+ meta = __glob_pattern_type (char_array_str (&dirname),
+ !(flags & GLOB_NOESCAPE));
/* 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) == '\\')
{
/* "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);
}
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);
@@ -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;
}
}
@@ -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;
}
++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;
/* 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);
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))
{
globfree (pglob);
pglob->gl_pathc = 0;
- retval = GLOB_NOSPACE;
- goto out;
+ goto err_nospace;
}
}
}
@@ -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;
}
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;
}
#if defined _LIBC && !defined __glob
versioned_symbol (libc, __glob, glob, GLIBC_2_27);
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-03-24 17:39 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-21 13:55 [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 6/8] posix: Remove alloca usage on glob user_name Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 7/8] posix: Use char_array for home_dir in glob Adhemerval Zanella
2017-11-21 14:24 ` Andreas Schwab
2017-11-21 13:55 ` [PATCH 2/8] posix: Use char_array for internal glob dirname Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 3/8] posix: Remove alloca usage for GLOB_BRACE on glob Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 8/8] posix: Remove all alloca usage in glob Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 5/8] posix: Use dynarray for globname " Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 4/8] posix: Remove alloca usage on glob dirname Adhemerval Zanella
2017-11-21 13:55 ` [PATCH 1/8] malloc: Add specialized dynarray for C strings Adhemerval Zanella
2017-11-21 14:15 ` Andreas Schwab
2017-11-21 16:20 ` Adhemerval Zanella
2017-11-21 14:19 ` Andreas Schwab
2017-11-21 16:32 ` Adhemerval Zanella
2017-12-12 17:02 ` [PATCH 0/8] posix: glob fixes and refactor Adhemerval Zanella
2021-01-05 18:58 [PATCH 0/8] Remove alloca usage from glob Adhemerval Zanella
2021-01-05 18:58 ` [PATCH 2/8] posix: Use char_array for internal glob dirname Adhemerval Zanella
2021-03-23 16:08 ` Arjun Shankar
2021-03-24 17:39 ` Adhemerval Zanella
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 read-only IMAP folder(s) and NNTP newsgroup(s).