From: Herbert Xu <herbert@gondor.apana.org.au>
To: Harald van Dijk <harald@gigawatt.nl>
Cc: calestyo@scientia.org, dash@vger.kernel.org
Subject: [PATCH] parser: Add VSBIT to ensure subtype is never zero
Date: Tue, 6 Dec 2022 13:56:38 +0800 [thread overview]
Message-ID: <Y47ZlpwkQy+jiule@gondor.apana.org.au> (raw)
In-Reply-To: <8710d1c3-d7c9-7332-4bc7-ce243a1cbd37@gigawatt.nl>
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
next prev parent reply other threads:[~2022-12-06 5:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Herbert Xu [this message]
2022-12-06 15:19 ` [PATCH] parser: Add VSBIT to ensure subtype is never zero Christoph Anton Mitterer
2022-12-07 4:03 ` Herbert Xu
2022-12-07 8:48 ` [v2 PATCH] " Herbert Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y47ZlpwkQy+jiule@gondor.apana.org.au \
--to=herbert@gondor.apana.org.au \
--cc=calestyo@scientia.org \
--cc=dash@vger.kernel.org \
--cc=harald@gigawatt.nl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).