dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* segfault with invalid shell script
@ 2022-11-21  2:38 Christoph Anton Mitterer
  2022-11-21 13:08 ` Harald van Dijk
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Anton Mitterer @ 2022-11-21  2:38 UTC (permalink / raw)
  To: dash

Hey.


I found the following issue by chance, when converting a shell
script[0] from bash to POSIX sh (well that + the use of "local"):

Below is a strongly reduced version of [0] which still causes the
error:
-------------------------------------------------------------------
#!/bin/sh


reject_and_die()
{
    exit 1
}


reject_filtered_cmd()
{
    reject_and_die "disallowed command${restrict_path_list:+ (restrict-path: \"${restrict_path_list//|/\", \"}\")}"
}

reject_filtered_cmd
-------------------------------------------------------------------

As you can see, I missed one bashism, namely the
${parameter//pattern/string} form of parameter expansion.


Now executing this with dash:
$ dpkg -l dash | grep ^ii
ii  dash           0.5.11+git20210903+057cd650a4ed-9 amd64        POSIX-compliant shell
$ dash ssh_filter_btrbk.sh 
Segmentation fault
$

With kernel log:
Nov 21 03:31:37 heisenberg kernel: dash[145217]: segfault at 1 ip 000055fa32ef8cd4 sp 00007ffd79a75140 error 4 in dash[55fa32ef3000+13000]
Nov 21 03:31:37 heisenberg kernel: Code: e2 01 4c 8d 34 42 48 8d 05 61 d9 00 00 49 01 c6 89 f0 83 e0 02 89 85 fc fe ff ff 74 17 c7 85 fc fe ff ff 00 00 00 00 83 e3 fd <41> 80 3f 7e 0f 84 c2 05 00 00 48 8b 35 8b 58 01 00 48 8b 05 5c 53


Shouldn't that rather give some parsing error?



One some other system (where I cannot really test any further since I
have no root) it even may have caused some more:
[10527194.157467] ssh_filter_btrb[816610]: segfault at 0 ip 000055c8ac34a698 sp 00007ffd4a997080 error 4 in dash[55c8ac344000+13000]
[10527194.157482] Code: 85 c4 01 00 00 48 83 c4 68 4c 89 f8 5b 5d 41 5c 41 5d 41 5e 41 5f c3 0f 1f 00 48 83 c2 02 eb a2 66 90 4c 89 fa 4d 85 f6 78 98 <48> 8b 36 bf 01 00 00 00 eb 8e 66 0f 1f 44 00 00 0f b6 42 01 48 83
[10527195.790531] traps: pool-tracker-st[816482] trap int3 ip:7f94e8271295 sp:7f94deffc770 error:0 in libglib-2.0.so.0.6400.6[7f94e8235000+84000]

Not sure whether that traps is in anyway related or just some
coincidence.



Thanks,
Chris.


[0] https://github.com/digint/btrbk/blob/master/ssh_filter_btrbk.sh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-21  2:38 segfault with invalid shell script Christoph Anton Mitterer
@ 2022-11-21 13:08 ` Harald van Dijk
  2022-11-21 15:24   ` Christoph Anton Mitterer
  2022-11-23  2:20   ` Harald van Dijk
  0 siblings, 2 replies; 15+ messages in thread
From: Harald van Dijk @ 2022-11-21 13:08 UTC (permalink / raw)
  To: Christoph Anton Mitterer, dash

Hi,

On 21/11/2022 02:38, Christoph Anton Mitterer wrote:
> Hey.
> 
> 
> I found the following issue by chance, when converting a shell
> script[0] from bash to POSIX sh (well that + the use of "local"):
> 
> Below is a strongly reduced version of [0] which still causes the
> error:
> -------------------------------------------------------------------
> #!/bin/sh
> 
> 
> reject_and_die()
> {
>      exit 1
> }
> 
> 
> reject_filtered_cmd()
> {
>      reject_and_die "disallowed command${restrict_path_list:+ (restrict-path: \"${restrict_path_list//|/\", \"}\")}"
> }
> 
> reject_filtered_cmd
> -------------------------------------------------------------------
> 
> As you can see, I missed one bashism, namely the
> ${parameter//pattern/string} form of parameter expansion.

Right. This is intentionally accepted, like all non-standard 
substitutions, during parsing, but is supposed to raise a substitution 
error at runtime, so that scripts can do

   if shell_supports_subst; then
     echo ${var//a/b}
   else
     echo $var | sed s/a/b/g
   fi

Parsing this if statement requires parsing both sides of the branch, 
even ${var//a/b} will never be evaluated.

Actually performing this substitution should result in an error:

   $ dash -c 'echo ${$//1/2}'
   dash: 1: Bad substitution

> Now executing this with dash:
> $ dpkg -l dash | grep ^ii
> ii  dash           0.5.11+git20210903+057cd650a4ed-9 amd64        POSIX-compliant shell
> $ dash ssh_filter_btrbk.sh
> Segmentation fault
> $
> 
> With kernel log:
> Nov 21 03:31:37 heisenberg kernel: dash[145217]: segfault at 1 ip 000055fa32ef8cd4 sp 00007ffd79a75140 error 4 in dash[55fa32ef3000+13000]
> Nov 21 03:31:37 heisenberg kernel: Code: e2 01 4c 8d 34 42 48 8d 05 61 d9 00 00 49 01 c6 89 f0 83 e0 02 89 85 fc fe ff ff 74 17 c7 85 fc fe ff ff 00 00 00 00 83 e3 fd <41> 80 3f 7e 0f 84 c2 05 00 00 48 8b 35 8b 58 01 00 48 8b 05 5c 53
> 
> 
> Shouldn't that rather give some parsing error?

What the intended behaviour here is though, is unclear. The substitution 
containing ${...//...} is evaluated but the ${...//...} is skipped 
because $restrict_path_list is unset.

This should either result in the ${...//...} being skipped, or the "Bad 
substitution" error. Currently, what happens instead is it attempts, but 
fails, to skip the ${...//...}.

Fixing this so it produces "Bad substitution" should be easy, almost 
trivial. Fixing this so it skips the substitution is probably more 
complicated, but would probably better reflect the intent of the current 
code.

Cheers,
Harald van Dijk

> One some other system (where I cannot really test any further since I
> have no root) it even may have caused some more:
> [10527194.157467] ssh_filter_btrb[816610]: segfault at 0 ip 000055c8ac34a698 sp 00007ffd4a997080 error 4 in dash[55c8ac344000+13000]
> [10527194.157482] Code: 85 c4 01 00 00 48 83 c4 68 4c 89 f8 5b 5d 41 5c 41 5d 41 5e 41 5f c3 0f 1f 00 48 83 c2 02 eb a2 66 90 4c 89 fa 4d 85 f6 78 98 <48> 8b 36 bf 01 00 00 00 eb 8e 66 0f 1f 44 00 00 0f b6 42 01 48 83
> [10527195.790531] traps: pool-tracker-st[816482] trap int3 ip:7f94e8271295 sp:7f94deffc770 error:0 in libglib-2.0.so.0.6400.6[7f94e8235000+84000]
> 
> Not sure whether that traps is in anyway related or just some
> coincidence.
> 
> 
> 
> Thanks,
> Chris.
> 
> 
> [0] https://github.com/digint/btrbk/blob/master/ssh_filter_btrbk.sh
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-21 13:08 ` Harald van Dijk
@ 2022-11-21 15:24   ` Christoph Anton Mitterer
  2022-11-21 22:25     ` Harald van Dijk
  2022-11-23  2:20   ` Harald van Dijk
  1 sibling, 1 reply; 15+ messages in thread
From: Christoph Anton Mitterer @ 2022-11-21 15:24 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: dash

Hey Harald

On Mon, 2022-11-21 at 13:08 +0000, Harald van Dijk wrote:
> 
> This is intentionally accepted, like all non-standard 
> substitutions, during parsing, but is supposed to raise a
> substitution 
> error at runtime, so that scripts can do
> 
>    if shell_supports_subst; then
>      echo ${var//a/b}
>    else
>      echo $var | sed s/a/b/g
>    fi
> 
> Parsing this if statement requires parsing both sides of the branch, 
> even ${var//a/b} will never be evaluated.

Well, while I can understand the merit of this... it may also have some
drawbacks.


> Actually performing this substitution should result in an error:
> 
>    $ dash -c 'echo ${$//1/2}'
>    dash: 1: Bad substitution

It does here, too.

> What the intended behaviour here is though, is unclear. The
> substitution 
> containing ${...//...} is evaluated but the ${...//...} is skipped 
> because $restrict_path_list is unset.

In the original script, it may be set. I just shortened it.


> Fixing this so it produces "Bad substitution" should be easy, almost 
> trivial.

I, personally, would like that anyway better.

Conditionally choosing the variant of shell command language during
runtime seems quite fragile.
I'd rather see straight away, that I use something non-supported.



Is there some bugtracker for dash, so that it doesn't get forgotten?


Thanks,
Chris.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-21 15:24   ` Christoph Anton Mitterer
@ 2022-11-21 22:25     ` Harald van Dijk
  2022-11-22 13:59       ` Christoph Anton Mitterer
  0 siblings, 1 reply; 15+ messages in thread
From: Harald van Dijk @ 2022-11-21 22:25 UTC (permalink / raw)
  To: Christoph Anton Mitterer; +Cc: dash

Hi,

On 21/11/2022 15:24, Christoph Anton Mitterer wrote:
> Hey Harald
> 
> On Mon, 2022-11-21 at 13:08 +0000, Harald van Dijk wrote:
>>
>> This is intentionally accepted, like all non-standard
>> substitutions, during parsing, but is supposed to raise a
>> substitution
>> error at runtime, so that scripts can do
>>
>>     if shell_supports_subst; then
>>       echo ${var//a/b}
>>     else
>>       echo $var | sed s/a/b/g
>>     fi
>>
>> Parsing this if statement requires parsing both sides of the branch,
>> even ${var//a/b} will never be evaluated.
> 
> Well, while I can understand the merit of this... it may also have some
> drawbacks.
> 
> 
>> Actually performing this substitution should result in an error:
>>
>>     $ dash -c 'echo ${$//1/2}'
>>     dash: 1: Bad substitution
> 
> It does here, too.
> 
>> What the intended behaviour here is though, is unclear. The
>> substitution
>> containing ${...//...} is evaluated but the ${...//...} is skipped
>> because $restrict_path_list is unset.
> 
> In the original script, it may be set. I just shortened it.

Sure, but you will not hit this bug if it is set: if it is set, you will 
get the Bad substitution error that is expected for that.

>> Fixing this so it produces "Bad substitution" should be easy, almost
>> trivial.
> 
> I, personally, would like that anyway better.
> 
> Conditionally choosing the variant of shell command language during
> runtime seems quite fragile.
> I'd rather see straight away, that I use something non-supported.

That is reasonable and some shells do issue errors during parsing for 
the example I posted earlier. POSIX explicitly makes it unspecified what 
happens here; dash has gone the other way.

> Is there some bugtracker for dash, so that it doesn't get forgotten?

There is a patch tracker, 
<https://patchwork.kernel.org/project/dash/list/>, but I am not aware of 
a bug tracker.

Cheers,
Harald van Dijk

> Thanks,
> Chris.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-21 22:25     ` Harald van Dijk
@ 2022-11-22 13:59       ` Christoph Anton Mitterer
  0 siblings, 0 replies; 15+ messages in thread
From: Christoph Anton Mitterer @ 2022-11-22 13:59 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: dash

On Mon, 2022-11-21 at 22:25 +0000, Harald van Dijk wrote:
> > 
> There is a patch tracker, 
> <https://patchwork.kernel.org/project/dash/list/>, but I am not aware
> of 
> a bug tracker.

For now I've also filed it downstream in the Debian bug tracker:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1024635

just that it doesn't get accidentally forgotten until it's fixed.

Thanks,
Chris.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-21 13:08 ` Harald van Dijk
  2022-11-21 15:24   ` Christoph Anton Mitterer
@ 2022-11-23  2:20   ` Harald van Dijk
  2022-11-23  4:04     ` Christoph Anton Mitterer
  2022-12-06  5:56     ` [PATCH] parser: Add VSBIT to ensure subtype is never zero Herbert Xu
  1 sibling, 2 replies; 15+ messages in thread
From: Harald van Dijk @ 2022-11-23  2:20 UTC (permalink / raw)
  To: Christoph Anton Mitterer, dash

On 21/11/2022 13:08, Harald van Dijk wrote:
> On 21/11/2022 02:38, Christoph Anton Mitterer wrote:
>> reject_filtered_cmd()
>> {
>>      reject_and_die "disallowed command${restrict_path_list:+ 
>> (restrict-path: \"${restrict_path_list//|/\", \"}\")}"
>> }
>>
>> reject_filtered_cmd
>[...]
> This should either result in the ${...//...} being skipped, or the "Bad 
> substitution" error. Currently, what happens instead is it attempts, but 
> fails, to skip the ${...//...}.

The reason it fails is because the word is cut off.

Variable substitutions are encoded as a CTLVAR special character, 
followed by a byte indicating the type of substitution, followed by the 
rest of the substitution data. The type of substitution is the VSNORMAL, 
VSMINUS, etc. seen in parser.h. An invalid substitution is encoded as a 
value of 0.

When we define a function, we clone the function body in order to 
preserve it. Cloning the function body is done by cloning each node. 
Cloning a "word" node (NARG) involves copying the characters that make 
up the word up to and including the terminating null byte.

These two interact badly. The invalid substitution is seen as 
terminating the word, the rest of the word is not copied, but the 
expansion code does not have any way of seeing that anything got cut off 
and happily continues attempting to process the rest of the word.

If dash decides to issue an error in this case, this is not a problem: 
the null byte is guaranteed to be copied, and if processing is 
guaranteed to stop if a null byte is encountered, everything works out.

If dash decides to not issue an error in this case, the encoding of bad 
substitutions needs to change to a non-null byte. It appears that if we 
set the byte to VSNUL, the expansion logic is already able to handle it, 
but I have not tested this extensively.

Cheers,
Harald van Dijk

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-23  2:20   ` Harald van Dijk
@ 2022-11-23  4:04     ` Christoph Anton Mitterer
  2022-11-23 10:54       ` Harald van Dijk
  2022-12-06  5:56     ` [PATCH] parser: Add VSBIT to ensure subtype is never zero Herbert Xu
  1 sibling, 1 reply; 15+ messages in thread
From: Christoph Anton Mitterer @ 2022-11-23  4:04 UTC (permalink / raw)
  To: Harald van Dijk, dash

Hey.

Wasn't busxybox' sh (and that of klibc-utils) based on dash/ash as
well?

Tried it with them, but there I couldn't reproduce the issue.

Thanks,
Chris.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-23  4:04     ` Christoph Anton Mitterer
@ 2022-11-23 10:54       ` Harald van Dijk
  2022-11-23 17:21         ` Christoph Anton Mitterer
  0 siblings, 1 reply; 15+ messages in thread
From: Harald van Dijk @ 2022-11-23 10:54 UTC (permalink / raw)
  To: Christoph Anton Mitterer, dash

On 23/11/2022 04:04, Christoph Anton Mitterer wrote:
> Hey.
> 
> Wasn't busxybox' sh (and that of klibc-utils) based on dash/ash as
> well?

Yes, it is.

> Tried it with them, but there I couldn't reproduce the issue.

With the busybox I have on my own system, I ran reproduce it with:

   busybox ash -c 'f() { echo ${PWD-${PWD!}}; }; f'

This segfaults.

If you try it with ${...//...}, busybox added code to implement that, so 
it does not hit the "Bad substitution" logic.

Cheers,
Harald van Dijk

> Thanks,
> Chris.
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-23 10:54       ` Harald van Dijk
@ 2022-11-23 17:21         ` Christoph Anton Mitterer
  2022-11-23 23:30           ` Harald van Dijk
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Anton Mitterer @ 2022-11-23 17:21 UTC (permalink / raw)
  To: Harald van Dijk, dash

On Wed, 2022-11-23 at 10:54 +0000, Harald van Dijk wrote:
> With the busybox I have on my own system, I ran reproduce it with:
> 
>    busybox ash -c 'f() { echo ${PWD-${PWD!}}; }; f'
> 
> This segfaults.

Would you mind reporting this at busybox and klib-utils sh then?

I mean I could do, if you want me to, ... but since this was rather
found by you now, you should take the "credit".


Thanks,
Chris.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-23 17:21         ` Christoph Anton Mitterer
@ 2022-11-23 23:30           ` Harald van Dijk
  2022-11-24  5:18             ` Christoph Anton Mitterer
  0 siblings, 1 reply; 15+ messages in thread
From: Harald van Dijk @ 2022-11-23 23:30 UTC (permalink / raw)
  To: Christoph Anton Mitterer, dash

On 23/11/2022 17:21, Christoph Anton Mitterer wrote:
> On Wed, 2022-11-23 at 10:54 +0000, Harald van Dijk wrote:
>> With the busybox I have on my own system, I ran reproduce it with:
>>
>>     busybox ash -c 'f() { echo ${PWD-${PWD!}}; }; f'
>>
>> This segfaults.
> 
> Would you mind reporting this at busybox and klib-utils sh then?

Sure, I have reported this to the busybox list and CCed you. If there is 
a separate list for klib-utils where this should also be reported (I am 
not aware of one), could you forward it there?

Cheers,
Harald van Dijk

> I mean I could do, if you want me to, ... but since this was rather
> found by you now, you should take the "credit".
> 
> 
> Thanks,
> Chris.
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: segfault with invalid shell script
  2022-11-23 23:30           ` Harald van Dijk
@ 2022-11-24  5:18             ` Christoph Anton Mitterer
  0 siblings, 0 replies; 15+ messages in thread
From: Christoph Anton Mitterer @ 2022-11-24  5:18 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: dash

On Wed, 2022-11-23 at 23:30 +0000, Harald van Dijk wrote:
> Sure, I have reported this to the busybox list and CCed you.

Thanks, they'd also have a bugtracker:
https://bugs.busybox.net/


>  If there is 
> a separate list for klib-utils where this should also be reported (I
> am 
> not aware of one), could you forward it there?

There is:
https://git.kernel.org/pub/scm/libs/klibc/klibc.git/tree/README

I've forwarded it there (with you in CC):
https://lists.zytor.com/archives/klibc/2022-November/004694.html
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1024735

Thanks,
Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] parser: Add VSBIT to ensure subtype is never zero
  2022-11-23  2:20   ` Harald van Dijk
  2022-11-23  4:04     ` Christoph Anton Mitterer
@ 2022-12-06  5:56     ` Herbert Xu
  2022-12-06 15:19       ` Christoph Anton Mitterer
  1 sibling, 1 reply; 15+ messages in thread
From: Herbert Xu @ 2022-12-06  5:56 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: calestyo, dash

Harald van Dijk <harald@gigawatt.nl> wrote:
> On 21/11/2022 13:08, Harald van Dijk wrote:
>> On 21/11/2022 02:38, Christoph Anton Mitterer wrote:
>>> reject_filtered_cmd()
>>> {
>>>      reject_and_die "disallowed command${restrict_path_list:+ 
>>> (restrict-path: \"${restrict_path_list//|/\", \"}\")}"
>>> }
>>>
>>> reject_filtered_cmd
>>[...]
>> This should either result in the ${...//...} being skipped, or the "Bad 
>> substitution" error. Currently, what happens instead is it attempts, but 
>> fails, to skip the ${...//...}.
> 
> The reason it fails is because the word is cut off.
> 
> Variable substitutions are encoded as a CTLVAR special character, 
> followed by a byte indicating the type of substitution, followed by the 
> rest of the substitution data. The type of substitution is the VSNORMAL, 
> VSMINUS, etc. seen in parser.h. An invalid substitution is encoded as a 
> value of 0.
> 
> When we define a function, we clone the function body in order to 
> preserve it. Cloning the function body is done by cloning each node. 
> Cloning a "word" node (NARG) involves copying the characters that make 
> up the word up to and including the terminating null byte.
> 
> These two interact badly. The invalid substitution is seen as 
> terminating the word, the rest of the word is not copied, but the 
> expansion code does not have any way of seeing that anything got cut off 
> and happily continues attempting to process the rest of the word.
> 
> If dash decides to issue an error in this case, this is not a problem: 
> the null byte is guaranteed to be copied, and if processing is 
> guaranteed to stop if a null byte is encountered, everything works out.
> 
> If dash decides to not issue an error in this case, the encoding of bad 
> substitutions needs to change to a non-null byte. It appears that if we 
> set the byte to VSNUL, the expansion logic is already able to handle it, 
> but I have not tested this extensively.

Thanks for the analysis Harald!

This patch does basically what you've described except it uses a new
bit to avoid any confusion with a genuine VSNUL.

Fixes: 3df3edd13389 ("[PARSER] Report substition errors at...")
Reported-by: Christoph Anton Mitterer <calestyo@scientia.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/expand.c b/src/expand.c
index aea5cc4..6bb1216 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -701,7 +701,7 @@ evalvar(char *p, int flag)
 	int discard;
 	int quoted;
 
-	varflags = *p++;
+	varflags = *p++ & ~VSBIT;
 	subtype = varflags & VSTYPE;
 
 	quoted = flag & EXP_QUOTED;
diff --git a/src/parser.c b/src/parser.c
index 13c2df5..b26f34a 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1333,7 +1333,7 @@ badsub:
 			synstack->dblquote = newsyn != BASESYNTAX;
 		}
 
-		*((char *)stackblock() + typeloc) = subtype;
+		*((char *)stackblock() + typeloc) = subtype | VSBIT;
 		if (subtype != VSNORMAL) {
 			synstack->varnest++;
 			if (synstack->dblquote)
diff --git a/src/parser.h b/src/parser.h
index 7d2749b..729c15c 100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -50,6 +50,7 @@
 /* variable substitution byte (follows CTLVAR) */
 #define VSTYPE	0x0f		/* type of variable substitution */
 #define VSNUL	0x10		/* colon--treat the empty string as unset */
+#define VSBIT	0x20		/* Ensure subtype is not zero */
 
 /* values of VSTYPE field */
 #define VSNORMAL	0x1		/* normal variable:  $var or ${var} */

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] parser: Add VSBIT to ensure subtype is never zero
  2022-12-06  5:56     ` [PATCH] parser: Add VSBIT to ensure subtype is never zero Herbert Xu
@ 2022-12-06 15:19       ` Christoph Anton Mitterer
  2022-12-07  4:03         ` Herbert Xu
  2022-12-07  8:48         ` [v2 PATCH] " Herbert Xu
  0 siblings, 2 replies; 15+ messages in thread
From: Christoph Anton Mitterer @ 2022-12-06 15:19 UTC (permalink / raw)
  To: Herbert Xu, Harald van Dijk; +Cc: dash

Hey.

Is that already the final version of the patch? Cause then we could
ping klibc sh / busybox about it.


Thanks,
Chris.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] parser: Add VSBIT to ensure subtype is never zero
  2022-12-06 15:19       ` Christoph Anton Mitterer
@ 2022-12-07  4:03         ` Herbert Xu
  2022-12-07  8:48         ` [v2 PATCH] " Herbert Xu
  1 sibling, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2022-12-07  4:03 UTC (permalink / raw)
  To: Christoph Anton Mitterer; +Cc: Harald van Dijk, dash

On Tue, Dec 06, 2022 at 04:19:39PM +0100, Christoph Anton Mitterer wrote:
> 
> Is that already the final version of the patch? Cause then we could
> ping klibc sh / busybox about it.

Yes I will commit this to git soon.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [v2 PATCH] parser: Add VSBIT to ensure subtype is never zero
  2022-12-06 15:19       ` Christoph Anton Mitterer
  2022-12-07  4:03         ` Herbert Xu
@ 2022-12-07  8:48         ` Herbert Xu
  1 sibling, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2022-12-07  8:48 UTC (permalink / raw)
  To: Christoph Anton Mitterer; +Cc: Harald van Dijk, dash

On Tue, Dec 06, 2022 at 04:19:39PM +0100, Christoph Anton Mitterer wrote:
> 
> Is that already the final version of the patch? Cause then we could
> ping klibc sh / busybox about it.

Sorry, turns out that the patch was buggy.  Here is an update:

---8<---
Harald van Dijk <harald@gigawatt.nl> wrote:
> On 21/11/2022 13:08, Harald van Dijk wrote:
>> On 21/11/2022 02:38, Christoph Anton Mitterer wrote:
>>> reject_filtered_cmd()
>>> {
>>> ���� reject_and_die "disallowed command${restrict_path_list:+ 
>>> (restrict-path: \"${restrict_path_list//|/\", \"}\")}"
>>> }
>>>
>>> reject_filtered_cmd
>>[...]
>> This should either result in the ${...//...} being skipped, or the "Bad 
>> substitution" error. Currently, what happens instead is it attempts, but 
>> fails, to skip the ${...//...}.
> 
> The reason it fails is because the word is cut off.
> 
> Variable substitutions are encoded as a CTLVAR special character, 
> followed by a byte indicating the type of substitution, followed by the 
> rest of the substitution data. The type of substitution is the VSNORMAL, 
> VSMINUS, etc. seen in parser.h. An invalid substitution is encoded as a 
> value of 0.
> 
> When we define a function, we clone the function body in order to 
> preserve it. Cloning the function body is done by cloning each node. 
> Cloning a "word" node (NARG) involves copying the characters that make 
> up the word up to and including the terminating null byte.
> 
> These two interact badly. The invalid substitution is seen as 
> terminating the word, the rest of the word is not copied, but the 
> expansion code does not have any way of seeing that anything got cut off 
> and happily continues attempting to process the rest of the word.
> 
> If dash decides to issue an error in this case, this is not a problem: 
> the null byte is guaranteed to be copied, and if processing is 
> guaranteed to stop if a null byte is encountered, everything works out.
> 
> If dash decides to not issue an error in this case, the encoding of bad 
> substitutions needs to change to a non-null byte. It appears that if we 
> set the byte to VSNUL, the expansion logic is already able to handle it, 
> but I have not tested this extensively.

Thanks for the analysis Harald!

This patch does basically what you've described except it uses a new
bit to avoid any confusion with a genuine VSNUL.

Fixes: 3df3edd13389 ("[PARSER] Report substition errors at...")
Reported-by: Christoph Anton Mitterer <calestyo@scientia.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/expand.c b/src/expand.c
index fe6215a..2ed02d6 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -704,7 +704,7 @@ evalvar(char *p, int flag)
 	int discard;
 	int quoted;
 
-	varflags = *p++;
+	varflags = *p++ & ~VSBIT;
 	subtype = varflags & VSTYPE;
 
 	quoted = flag & EXP_QUOTED;
diff --git a/src/mystring.c b/src/mystring.c
index ed3c8f6..f651521 100644
--- a/src/mystring.c
+++ b/src/mystring.c
@@ -60,7 +60,7 @@
 char nullstr[1];		/* zero length string */
 const char spcstr[] = " ";
 const char snlfmt[] = "%s\n";
-const char dolatstr[] = { CTLQUOTEMARK, CTLVAR, VSNORMAL, '@', '=',
+const char dolatstr[] = { CTLQUOTEMARK, CTLVAR, VSNORMAL | VSBIT, '@', '=',
 			  CTLQUOTEMARK, '\0' };
 const char cqchars[] = {
 #ifdef HAVE_FNMATCH
diff --git a/src/parser.c b/src/parser.c
index bf94697..a552c47 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1333,7 +1333,7 @@ badsub:
 			synstack->dblquote = newsyn != BASESYNTAX;
 		}
 
-		*((char *)stackblock() + typeloc) = subtype;
+		*((char *)stackblock() + typeloc) = subtype | VSBIT;
 		if (subtype != VSNORMAL) {
 			synstack->varnest++;
 			if (synstack->dblquote)
diff --git a/src/parser.h b/src/parser.h
index 7d2749b..729c15c 100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -50,6 +50,7 @@
 /* variable substitution byte (follows CTLVAR) */
 #define VSTYPE	0x0f		/* type of variable substitution */
 #define VSNUL	0x10		/* colon--treat the empty string as unset */
+#define VSBIT	0x20		/* Ensure subtype is not zero */
 
 /* values of VSTYPE field */
 #define VSNORMAL	0x1		/* normal variable:  $var or ${var} */

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-12-07  8:52 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-21  2:38 segfault with invalid shell script Christoph Anton Mitterer
2022-11-21 13:08 ` Harald van Dijk
2022-11-21 15:24   ` Christoph Anton Mitterer
2022-11-21 22:25     ` Harald van Dijk
2022-11-22 13:59       ` Christoph Anton Mitterer
2022-11-23  2:20   ` Harald van Dijk
2022-11-23  4:04     ` Christoph Anton Mitterer
2022-11-23 10:54       ` Harald van Dijk
2022-11-23 17:21         ` Christoph Anton Mitterer
2022-11-23 23:30           ` Harald van Dijk
2022-11-24  5:18             ` Christoph Anton Mitterer
2022-12-06  5:56     ` [PATCH] parser: Add VSBIT to ensure subtype is never zero Herbert Xu
2022-12-06 15:19       ` Christoph Anton Mitterer
2022-12-07  4:03         ` Herbert Xu
2022-12-07  8:48         ` [v2 PATCH] " Herbert Xu

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 NNTP newsgroup(s).