Re: [PATCH v2] posix: Fix wordexp WRDE_APPEND to preserve state on non-NOSPACE errors (BZ 34090, CVE-2026-6368)

A few tests could have more coverage, and one question about a return
path.

Adhemerval Zanella <[email protected]> writes:
> +/* w_addword grows we_wordv with realloc, make every call guaranteed to
> +   relocate the block.  This makes BZ 34090 regression more deterministic.  */
> +void *
> +realloc (void *ptr, size_t size)
> +{
> +  if (ptr == NULL)
> +    return malloc (size);
> +  if (size == 0)
> +    {
> +      free (ptr);
> +      return NULL;
> +    }
> +
> +  void *new = malloc (size);
> +  if (new == NULL)
> +    return NULL;
> +
> +  /* Copy only what is valid in the old block to avoid reading past it.  */
> +  size_t old = malloc_usable_size (ptr);
> +  memcpy (new, ptr, old < size ? old : size);
> +  free (ptr);
> +  return new;
> +}

Ok.  Might be useful to fill the old chunk with junk before freeing, but
I don't see how it would help this test.

> +/* Verify that all words in we match the expected NULL-terminated
> +   array.  */
> +static void
> +check_words (const wordexp_t *we, const char *const *expected, int line)
> +{
> +  size_t i;
> +  for (i = 0; expected[i] != NULL; i++)
> +    {
> +      TEST_VERIFY (i < we->we_wordc);
> +      TEST_COMPARE_STRING (we->we_wordv[we->we_offs + i], expected[i]);
> +    }
> +  TEST_COMPARE (we->we_wordc, i);
> +}

What is the "line" argument here for?

> +#define CHECK_WORDS(we, ...) \
> +  do {								\
> +    const char *const expected_[] = { __VA_ARGS__, NULL };	\
> +    check_words (we, expected_, __LINE__);			\
> +  } while (0)

Ok.

> +/* Test 1: WRDE_APPEND + WRDE_BADCHAR preserves we_wordc.  */
> +static void
> +test_append_badchar_preserves_count (void)
> +{
> +  printf ("info: test_append_badchar_preserves_count\n");
> +  wordexp_t we = { 0 };
> +
> +  TEST_COMPARE (wordexp ("one two three", &we, 0), 0);
> +  TEST_COMPARE (we.we_wordc, 3);
> +
> +  size_t saved_count = we.we_wordc;
> +
> +  /* ')' triggers WRDE_BADCHAR and  "extra" would be a new word if the
> +     expansion succeeded, exercising the w_addword path before the error
> +     is detected.  */
> +  TEST_COMPARE (wordexp ("extra )", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (we.we_wordc, saved_count);
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Test 2: WRDE_APPEND + WRDE_BADCHAR preserves the we_wordv pointer even
> +   when internal realloc would move the buffer.  */
> +static void
> +test_append_badchar_preserves_pointer (void)
> +{
> +  printf ("info: test_append_badchar_preserves_pointer\n");
> +  wordexp_t we = { 0 };
> +
> +  /* Use many words so that the initial we_wordv allocation is
> +     non-trivial and a later realloc is more likely to move it.  */
> +  TEST_COMPARE (wordexp ("a b c d e f g h", &we, 0), 0);
> +  TEST_COMPARE (we.we_wordc, 8);
> +
> +  char **saved_wordv = we.we_wordv;
> +  size_t saved_count = we.we_wordc;
> +
> +  /* The interposed realloc guarantees the internal we_wordv buffer moves
> +     during parsing, so the pointer-stability check below is meaningful.  */
> +  TEST_COMPARE (wordexp ("append )", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (we.we_wordc, saved_count);
> +  TEST_VERIFY (we.we_wordv == saved_wordv);
> +
> +  wordfree (&we);
> +}

The test should verify that realloc was actually called.

> +/* Test 3: After a failed WRDE_APPEND the original words are still accessible
> +   and correct.  */
> +static void
> +test_append_badchar_words_intact (void)
> +{
> +  printf ("info: test_append_badchar_words_intact\n");
> +  wordexp_t we = { 0 };
> +
> +  TEST_COMPARE (wordexp ("alpha beta gamma", &we, 0), 0);
> +  CHECK_WORDS (&we, "alpha", "beta", "gamma");
> +
> +  TEST_COMPARE (wordexp ("delta )", &we, WRDE_APPEND), WRDE_BADCHAR);
> +
> +  /* Words must still be intact.  */
> +  CHECK_WORDS (&we, "alpha", "beta", "gamma");
> +  /* The NULL terminator must still be present.  */
> +  TEST_VERIFY (we.we_wordv[we.we_offs + we.we_wordc] == NULL);
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Test 4: Successful WRDE_APPEND still works (regression test).  */
> +static void
> +test_append_success (void)
> +{
> +  printf ("info: test_append_success\n");
> +  wordexp_t we = { 0 };
> +
> +  TEST_COMPARE (wordexp ("hello", &we, 0), 0);
> +  TEST_COMPARE (we.we_wordc, 1);
> +
> +  TEST_COMPARE (wordexp ("world", &we, WRDE_APPEND), 0);
> +  TEST_COMPARE (we.we_wordc, 2);
> +  CHECK_WORDS (&we, "hello", "world");
> +
> +  wordfree (&we);
> +}

Should test that the pointer actually changed, too.

> +/* Test 5: Successful append after a failed append — the implementation must
> +   recover and allow further use of the wordexp_t.  */
> +static void
> +test_append_success_after_failure (void)
> +{
> +  printf ("info: test_append_success_after_failure\n");
> +  wordexp_t we = { 0 };
> +
> +  TEST_COMPARE (wordexp ("first", &we, 0), 0);
> +  CHECK_WORDS (&we, "first");
> +
> +  TEST_COMPARE (wordexp ("bad |", &we, WRDE_APPEND), WRDE_BADCHAR);
> +
> +  /* State must be exactly as before the failed call.  */
> +  CHECK_WORDS (&we, "first");
> +
> +  /* A subsequent successful append must work.  */
> +  TEST_COMPARE (wordexp ("second third", &we, WRDE_APPEND), 0);
> +  CHECK_WORDS (&we, "first", "second", "third");
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Test 6: Multiple consecutive failed appends do not corrupt state.  */
> +static void
> +test_append_multiple_failures (void)
> +{
> +  printf ("info: test_append_multiple_failures\n");
> +  wordexp_t we = { 0 };
> +
> +  TEST_COMPARE (wordexp ("keep this", &we, 0), 0);
> +  CHECK_WORDS (&we, "keep", "this");
> +
> +  size_t saved_count = we.we_wordc;
> +  char **saved_wordv = we.we_wordv;
> +
> +  /* Each of these bad characters must leave the state unchanged.  */
> +  TEST_COMPARE (wordexp ("x )", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (wordexp ("x |", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (wordexp ("x ;", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (wordexp ("x &", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (wordexp ("x <", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (wordexp ("x >", &we, WRDE_APPEND), WRDE_BADCHAR);
> +
> +  TEST_COMPARE (we.we_wordc, saved_count);
> +  TEST_VERIFY (we.we_wordv == saved_wordv);
> +  CHECK_WORDS (&we, "keep", "this");
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Test 7: WRDE_APPEND with WRDE_SYNTAX error (unterminated quote) also
> +   preserves state.  */
> +static void
> +test_append_syntax_error (void)
> +{
> +  printf ("info: test_append_syntax_error\n");
> +  wordexp_t we = { 0 };
> +
> +  TEST_COMPARE (wordexp ("original", &we, 0), 0);
> +  CHECK_WORDS (&we, "original");
> +
> +  char **saved_wordv = we.we_wordv;
> +  size_t saved_count = we.we_wordc;
> +
> +  /* Unterminated double quote triggers WRDE_SYNTAX.  */
> +  TEST_COMPARE (wordexp ("\"unterminated", &we, WRDE_APPEND), WRDE_SYNTAX);
> +
> +  TEST_COMPARE (we.we_wordc, saved_count);
> +  TEST_VERIFY (we.we_wordv == saved_wordv);
> +  CHECK_WORDS (&we, "original");
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Test 8: Error without WRDE_APPEND still works (regression test for the
> +   non-APPEND code path in do_error).  */
> +static void
> +test_no_append_error (void)
> +{
> +  printf ("info: test_no_append_error\n");
> +  wordexp_t we = { 0 };
> +
> +  /* Simple failure without WRDE_APPEND.  */
> +  TEST_COMPARE (wordexp ("bad |", &we, 0), WRDE_BADCHAR);
> +
> +  /* After failure without WRDE_APPEND the struct should be safe to
> +     reuse — start fresh.  */
> +  TEST_COMPARE (wordexp ("ok", &we, 0), 0);
> +  CHECK_WORDS (&we, "ok");
> +
> +  wordfree (&we);
> +}

Should there be a test that "we" is unchanged?  I think the
wordexp/check is sufficient.  Ok.

> +/* Test 9: WRDE_BADCHAR on the very first character (no partial words added
> +   before the error).  */
> +static void
> +test_append_badchar_immediate (void)
> +{
> +  printf ("info: test_append_badchar_immediate\n");
> +  wordexp_t we = { 0 };
> +
> +  TEST_COMPARE (wordexp ("hello world", &we, 0), 0);
> +  CHECK_WORDS (&we, "hello", "world");
> +
> +  char **saved_wordv = we.we_wordv;
> +  size_t saved_count = we.we_wordc;
> +
> +  /* The bad character is the very first byte — no w_addword call happens
> +     before the error.  */
> +  TEST_COMPARE (wordexp ("|", &we, WRDE_APPEND), WRDE_BADCHAR);
> +  TEST_COMPARE (we.we_wordc, saved_count);
> +  TEST_VERIFY (we.we_wordv == saved_wordv);
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Test 10: WRDE_APPEND into an empty wordexp_t (initial call uses WRDE_APPEND
> +   with a zeroed struct — unusual but allowed).  */
> +static void
> +test_append_into_empty (void)
> +{
> +  printf ("info: test_append_into_empty\n");
> +  wordexp_t we = { 0 };
> +
> +  /* First call with WRDE_APPEND on a zeroed struct.  The implementation
> +     must handle we_wordv == NULL gracefully.  */
> +  TEST_COMPARE (wordexp ("solo", &we, WRDE_APPEND), 0);
> +  TEST_COMPARE (we.we_wordc, 1);
> +  CHECK_WORDS (&we, "solo");
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Verify that the leading we_offs slots are all NULL.  */
> +static void
> +check_offs_null (const wordexp_t *we)
> +{
> +  for (size_t i = 0; i < we->we_offs; i++)
> +    TEST_VERIFY (we->we_wordv[i] == NULL);
> +}

Ok.

> +/* Test 11: successful WRDE_APPEND with WRDE_DOOFFS and a non-zero we_offs.
> +   The leading offset slots must stay NULL and words must land at
> +   we_wordv[we_offs + i] across both the initial and the appended call.  */
> +static void
> +test_dooffs_append_success (void)
> +{
> +  printf ("info: test_dooffs_append_success\n");
> +  wordexp_t we = { 0 };
> +  we.we_offs = 2;
> +
> +  TEST_COMPARE (wordexp ("one two", &we, WRDE_DOOFFS), 0);
> +  TEST_COMPARE (we.we_offs, 2);
> +  check_offs_null (&we);
> +  CHECK_WORDS (&we, "one", "two");
> +
> +  TEST_COMPARE (wordexp ("three", &we, WRDE_APPEND | WRDE_DOOFFS), 0);
> +  TEST_COMPARE (we.we_offs, 2);
> +  check_offs_null (&we);
> +  CHECK_WORDS (&we, "one", "two", "three");
> +  /* The NULL terminator must sit right after the last word.  */
> +  TEST_VERIFY (we.we_wordv[we.we_offs + we.we_wordc] == NULL);
> +
> +  wordfree (&we);
> +}

Ok.

> +/* Test 12: failed WRDE_APPEND with WRDE_DOOFFS preserves we_wordc, the
> +   we_wordv pointer, the words and the leading NULL offset slots.  This
> +   exercises the we_offs arithmetic in the array duplication and in the
> +   error-path cleanup (we_wordv[we_offs + --we_wordc]).  */
> +static void
> +test_dooffs_append_error_preserves_state (void)
> +{
> +  printf ("info: test_dooffs_append_error_preserves_state\n");
> +  wordexp_t we = { 0 };
> +  we.we_offs = 3;
> +
> +  TEST_COMPARE (wordexp ("alpha beta", &we, WRDE_DOOFFS), 0);
> +  check_offs_null (&we);
> +  CHECK_WORDS (&we, "alpha", "beta");
> +
> +  char **saved_wordv = we.we_wordv;
> +  size_t saved_count = we.we_wordc;
> +
> +  /* "gamma" is a partial word added via w_addword (forcing a relocating
> +     realloc of we_wordv) before ')' triggers WRDE_BADCHAR.  */
> +  TEST_COMPARE (wordexp ("gamma )", &we, WRDE_APPEND | WRDE_DOOFFS),
> +		WRDE_BADCHAR);
> +
> +  TEST_COMPARE (we.we_offs, 3);
> +  TEST_COMPARE (we.we_wordc, saved_count);
> +  TEST_VERIFY (we.we_wordv == saved_wordv);
> +  check_offs_null (&we);
> +  CHECK_WORDS (&we, "alpha", "beta");
> +  TEST_VERIFY (we.we_wordv[we.we_offs + we.we_wordc] == NULL);
> +
> +  wordfree (&we);
> +}

Ok.


> +static int
> +do_test (void)
> +{
> +  test_append_badchar_preserves_count ();
> +  test_append_badchar_preserves_pointer ();
> +  test_append_badchar_words_intact ();
> +  test_append_success ();
> +  test_append_success_after_failure ();
> +  test_append_multiple_failures ();
> +  test_append_syntax_error ();
> +  test_no_append_error ();
> +  test_append_badchar_immediate ();
> +  test_append_into_empty ();
> +  test_dooffs_append_success ();
> +  test_dooffs_append_error_preserves_state ();
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Ok.

> diff --git a/posix/wordexp.c b/posix/wordexp.c

> +  /* When WRDE_APPEND is set we work on a copy of the we_wordv array so that
> +     the caller's original pointer is never invalidated by realloc inside
> +     w_addword.  The saved_wordv keeps the original; on success we free it,
> +     on non-NOSPACE error we free the working copy and restore the original.  */
> +  char **saved_wordv = NULL;

Ok.


> +  else if (pwordexp->we_wordv != NULL)
> +    {
> +      /* WRDE_APPEND with an existing word list: duplicate the array so that
> +	 realloc during parsing does not invalidate the caller's pointer.  The
> +	 strings themselves are shared.  */
> +      size_t num_p;
> +      char **dup;
> +      if (INT_ADD_WRAPV (pwordexp->we_offs, pwordexp->we_wordc, &num_p)
> +	  || INT_ADD_WRAPV (num_p, 1, &num_p))
> +	return WRDE_NOSPACE;
> +      dup = __libc_reallocarray (NULL, num_p, sizeof *dup);

I assume we rely on reallocarray to do its own overflow check, so...

> +      if (dup == NULL)
> +	return WRDE_NOSPACE;
> +      memcpy (dup, pwordexp->we_wordv, num_p * sizeof *dup);

We don't need to do it here.

> +      saved_wordv = pwordexp->we_wordv;
> +      pwordexp->we_wordv = dup;
> +    }

Ok.

>  	    error = w_addword (pwordexp, NULL);
>  
>  	    if (error)
> -	      return error;
> +	      goto do_error;
>  	  }

Ok.

>  	    if (error)
> -	      return error;
> +	      goto do_error;
>  	  }

Ok.

>  
>    /* There was a word separator at the end */
>    if (word == NULL) /* i.e. w_newword */
> -    return 0;
> +    {
> +      free (saved_wordv);
> +      return 0;
> +    }

Ok.

>    /* There was no field separator at the end */
> -  return w_addword (pwordexp, word);
> +  error = w_addword (pwordexp, word);
> +  free (saved_wordv);
> +  return error;

Is there a possible error here that would require us to preserve the
original array?

>    if (error == WRDE_NOSPACE)
> -    return WRDE_NOSPACE;
> +    {
> +      /* we_wordc and we_wordv are updated to reflect any words that were
> +	 successfully expanded.  The old array is obsolete.  */
> +      free (saved_wordv);
> +      return WRDE_NOSPACE;
> +    }

Ok.

> -  if ((flags & WRDE_APPEND) == 0)
> -    wordfree (pwordexp);
> +  if (flags & WRDE_APPEND)
> +    {
> +      /* POSIX 2024 states that for in other error cases, if the WRDE_APPEND
> +	 flag was specified, we_wordc and we_wordv shall not be modified.
> +
> +	 Free strings appended during this call, discard the working copy of
> +	 we_wordv, and restore the caller's original pointer.  */
> +      while (pwordexp->we_wordc > old_word.we_wordc)
> +	free (pwordexp->we_wordv[pwordexp->we_offs + --pwordexp->we_wordc]);
> +      free (pwordexp->we_wordv);
> +      pwordexp->we_wordv = saved_wordv;
> +    }
> +  else
> +    {
> +      wordfree (pwordexp);
> +      *pwordexp = old_word;
> +    }
>  
> -  *pwordexp = old_word;
>    return error;
>  }

Ok.