* [PATCH] posix: Fix some null deferences in wordexp [BZ #18096]
@ 2023-03-18 12:59 Julian Squires
2023-03-18 15:10 ` Andreas Schwab
2023-03-22 16:39 ` [PATCH v2] posix: Fix some crashes " Julian Squires
0 siblings, 2 replies; 6+ messages in thread
From: Julian Squires @ 2023-03-18 12:59 UTC (permalink / raw)
To: libc-alpha; +Cc: Julian Squires
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(-)
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;
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] posix: Fix some null deferences in wordexp [BZ #18096]
2023-03-18 12:59 [PATCH] posix: Fix some null deferences in wordexp [BZ #18096] Julian Squires
@ 2023-03-18 15:10 ` Andreas Schwab
2023-03-19 13:49 ` Julian Squires
2023-03-22 16:39 ` [PATCH v2] posix: Fix some crashes " Julian Squires
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Schwab @ 2023-03-18 15:10 UTC (permalink / raw)
To: Julian Squires via Libc-alpha; +Cc: Julian Squires
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?
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] posix: Fix some null deferences in wordexp [BZ #18096]
2023-03-18 15:10 ` Andreas Schwab
@ 2023-03-19 13:49 ` Julian Squires
2023-03-19 14:16 ` Andreas Schwab
0 siblings, 1 reply; 6+ messages in thread
From: Julian Squires @ 2023-03-19 13:49 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Julian Squires via Libc-alpha
[-- Attachment #1: Type: text/plain, Size: 587 bytes --]
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);
}
--
Julian Squires
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] posix: Fix some null deferences in wordexp [BZ #18096]
2023-03-19 13:49 ` Julian Squires
@ 2023-03-19 14:16 ` Andreas Schwab
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Schwab @ 2023-03-19 14:16 UTC (permalink / raw)
To: Julian Squires; +Cc: Julian Squires via Libc-alpha
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.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] posix: Fix some crashes in wordexp [BZ #18096]
2023-03-18 12:59 [PATCH] posix: Fix some null deferences in wordexp [BZ #18096] Julian Squires
2023-03-18 15:10 ` Andreas Schwab
@ 2023-03-22 16:39 ` Julian Squires
2023-03-28 13:09 ` Adhemerval Zanella Netto
1 sibling, 1 reply; 6+ messages in thread
From: Julian Squires @ 2023-03-22 16:39 UTC (permalink / raw)
To: libc-alpha; +Cc: Julian Squires
Without these fixes, the first three included tests segfault (on a
NULL dereference); the fourth aborts on an assertion, which is itself
unnecessary.
Signed-off-by: Julian Squires <julian@cipht.net>
---
v2: removed the assert under seen_hash as requested. Thanks for the
review.
posix/wordexp-test.c | 4 ++++
posix/wordexp.c | 11 ++++-------
2 files changed, 8 insertions(+), 7 deletions(-)
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..b34c4a939b 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);
@@ -1883,10 +1883,7 @@ envsubst:
_itoa_word (value ? strlen (value) : 0,
¶m_length[20], 10, 0));
if (free_value)
- {
- assert (value != NULL);
- free (value);
- }
+ free (value);
return *word ? 0 : WRDE_NOSPACE;
}
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] posix: Fix some crashes in wordexp [BZ #18096]
2023-03-22 16:39 ` [PATCH v2] posix: Fix some crashes " Julian Squires
@ 2023-03-28 13:09 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 6+ messages in thread
From: Adhemerval Zanella Netto @ 2023-03-28 13:09 UTC (permalink / raw)
To: libc-alpha, Julian Squires
On 22/03/23 13:39, Julian Squires via Libc-alpha wrote:
> Without these fixes, the first three included tests segfault (on a
> NULL dereference); the fourth aborts on an assertion, which is itself
> unnecessary.
>
> Signed-off-by: Julian Squires <julian@cipht.net>
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> v2: removed the assert under seen_hash as requested. Thanks for the
> review.
>
> posix/wordexp-test.c | 4 ++++
> posix/wordexp.c | 11 ++++-------
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> 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 },
Ok, empty arithmetic expansion.
> + { 0, NULL, "$[]", 0, 1, { "0", }, IFS },
>
Ok, same as above (although with undocumented syntax not supported by all
shells).
> /* 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 },
Ok, parameter expansion.
>
> { 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..b34c4a939b 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);
> @@ -1883,10 +1883,7 @@ envsubst:
> _itoa_word (value ? strlen (value) : 0,
> ¶m_length[20], 10, 0));
> if (free_value)
> - {
> - assert (value != NULL);
> - free (value);
> - }
> + free (value);
>
> return *word ? 0 : WRDE_NOSPACE;
> }
Ok.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-28 13:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-18 12:59 [PATCH] posix: Fix some null deferences in wordexp [BZ #18096] Julian Squires
2023-03-18 15:10 ` Andreas Schwab
2023-03-19 13:49 ` Julian Squires
2023-03-19 14:16 ` Andreas Schwab
2023-03-22 16:39 ` [PATCH v2] posix: Fix some crashes " Julian Squires
2023-03-28 13:09 ` Adhemerval Zanella Netto
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).