diff mbox series

posix: Fix some null deferences in wordexp [BZ #18096]

Message ID 20230318125950.3611824-1-julian@cipht.net
State Changes Requested
Headers
Series posix: Fix some null deferences in wordexp [BZ #18096] |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Julian Squires March 18, 2023, 12:59 p.m. UTC 
  Without these fixes, the first three included tests segfault (on a
NULL dereference); the third aborts on an assertion.

Signed-off-by: Julian Squires <julian@cipht.net>
---
I wasn't aware of the long-languishing issue in Bugzilla before
starting this, which largely includes the same changes, but perhaps
supplying this with test cases will help it be adopted.  Despite the
security exception for wordexp, it still seems reasonable not to crash
in these cases.

 posix/wordexp-test.c |  4 ++++
 posix/wordexp.c      | 12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

Comments

Andreas Schwab March 18, 2023, 3:10 p.m. UTC | #1 
On Mär 18 2023, Julian Squires via Libc-alpha wrote:

> @@ -1813,7 +1813,7 @@ envsubst:
>  	    goto success;
>  
>  	  value = pattern ? __strdup (pattern) : pattern;
> -	  free_value = 1;
> +	  free_value = !!pattern;

What does that fix?
Julian Squires March 19, 2023, 1:49 p.m. UTC | #2 
Andreas Schwab <schwab@linux-m68k.org> writes:
> On Mär 18 2023, Julian Squires via Libc-alpha wrote:
>> @@ -1813,7 +1813,7 @@ envsubst:
>>  	    goto success;
>>  
>>  	  value = pattern ? __strdup (pattern) : pattern;
>> -	  free_value = 1;
>> +	  free_value = !!pattern;
>
> What does that fix?

The assertion failure mentioned, where seen_hash is set, triggering the
assertion below, where free_value is set but value is NULL:

  if (seen_hash)
    {
      [...]
      if (free_value)
	{
	  assert (value != NULL);
	  free (value);
	}
Andreas Schwab March 19, 2023, 2:16 p.m. UTC | #3 
On Mär 19 2023, Julian Squires wrote:

> The assertion failure mentioned, where seen_hash is set, triggering the
> assertion below, where free_value is set but value is NULL:
>
>   if (seen_hash)
>     {
>       [...]
>       if (free_value)
> 	{
> 	  assert (value != NULL);
> 	  free (value);
> 	}

That assertion should be removed, it serves no purpose.
diff mbox series

Patch

diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c
index f7a591149b..bae27d6cee 100644
--- a/posix/wordexp-test.c
+++ b/posix/wordexp-test.c
@@ -117,6 +117,8 @@  struct test_case_struct
     { 0, NULL, "$((010+0x10))", 0, 1, { "24" }, IFS },
     { 0, NULL, "$((-010+0x10))", 0, 1, { "8" }, IFS },
     { 0, NULL, "$((-0x10+010))", 0, 1, { "-8" }, IFS },
+    { 0, NULL, "$(())", 0, 1, { "0", }, IFS },
+    { 0, NULL, "$[]", 0, 1, { "0", }, IFS },
 
     /* Advanced parameter expansion */
     { 0, NULL, "${var:-bar}", 0, 1, { "bar", }, IFS },
@@ -138,6 +140,8 @@  struct test_case_struct
     { 0, "12345", "${#var}", 0, 1, { "5", }, IFS },
     { 0, NULL, "${var:-'}'}", 0, 1, { "}", }, IFS },
     { 0, NULL, "${var-}", 0, 0, { NULL }, IFS },
+    { 0, NULL, "${a?}", 0, 0, { NULL, }, IFS },
+    { 0, NULL, "${#a=}", 0, 1, { "0", }, IFS },
 
     { 0, "pizza", "${var#${var}}", 0, 0, { NULL }, IFS },
     { 0, "pepperoni", "${var%$(echo oni)}", 0, 1, { "pepper" }, IFS },
diff --git a/posix/wordexp.c b/posix/wordexp.c
index 0da98f5b08..287bb05feb 100644
--- a/posix/wordexp.c
+++ b/posix/wordexp.c
@@ -720,7 +720,7 @@  parse_arith (char **word, size_t *word_length, size_t *max_length,
 	      ++(*offset);
 
 	      /* Go - evaluate. */
-	      if (*expr && eval_expr (expr, &numresult) != 0)
+	      if (expr && eval_expr (expr, &numresult) != 0)
 		{
 		  free (expr);
 		  return WRDE_SYNTAX;
@@ -758,7 +758,7 @@  parse_arith (char **word, size_t *word_length, size_t *max_length,
 	      long int numresult = 0;
 
 	      /* Go - evaluate. */
-	      if (*expr && eval_expr (expr, &numresult) != 0)
+	      if (expr && eval_expr (expr, &numresult) != 0)
 		{
 		  free (expr);
 		  return WRDE_SYNTAX;
@@ -1790,7 +1790,7 @@  envsubst:
 	    {
 	      const char *str = pattern;
 
-	      if (str[0] == '\0')
+	      if (!str || str[0] == '\0')
 		str = _("parameter null or not set");
 
 	      __fxprintf (NULL, "%s: %s\n", env, str);
@@ -1813,7 +1813,7 @@  envsubst:
 	    goto success;
 
 	  value = pattern ? __strdup (pattern) : pattern;
-	  free_value = 1;
+	  free_value = !!pattern;
 
 	  if (pattern && !value)
 	    goto no_space;
@@ -1827,7 +1827,7 @@  envsubst:
 		free (value);
 
 	      value = pattern ? __strdup (pattern) : pattern;
-	      free_value = 1;
+	      free_value = !!pattern;
 
 	      if (pattern && !value)
 		goto no_space;
@@ -1857,7 +1857,7 @@  envsubst:
 	    free (value);
 
 	  value = pattern ? __strdup (pattern) : pattern;
-	  free_value = 1;
+	  free_value = !!pattern;
 
 	  if (pattern && !value)
 	    goto no_space;