parser: Add VSBIT to ensure subtype is never zero

Commit Message

Herbert Xu Dec. 6, 2022, 5:56 a.m. UTC

Harald van Dijk <[e-mail]> 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 <[e-mail]>
Signed-off-by: Herbert Xu <[e-mail]>


Thanks,

Comments

Christoph Anton Mitterer Dec. 6, 2022, 3:19 p.m. UTC | #1

Hey.

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


Thanks,
Chris.

Herbert Xu Dec. 7, 2022, 4:03 a.m. UTC | #2

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,

Patch

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} */