On 3/26/21 3:21 PM, Paul Eggert wrote:
The -S code could use some more fixes in this area too - it can
probably still dump core on platforms like the Hurd that don't limit
exec arg size - but one thing at a time.
I fixed the (unlikely) bugs I found in this area by installing the attached.
>From e3766c5db176ca7abbb8212d5b0b7862fb98a5be Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Mon, 29 Mar 2021 21:42:44 -0700
Subject: [PATCH] env: simplify --split-string memory management
* bootstrap.conf (gnulib_modules): Add idx.
* src/env.c: Include idx.h, minmax.h.
Prefer idx_t to ptrdiff_t when values are nonnegative.
(valid_escape_sequence, escape_char, validate_split_str)
(CHECK_START_NEW_ARG):
Remove; no longer needed now that we validate as we go.
(struct splitbuf): New type.
(splitbuf_grow, splitbuf_append_byte, check_start_new_arg)
(splitbuf_finishup): New functions.
(build_argv): New arg ARGC. Validate and process in one go, using
the new functions; this is simpler and more reliable than the old
approach (as witness the recent bug). Avoid integer overflow in
the unlikely case where the string contains more than INT_MAX
arguments.
(parse_split_string): Simplify by exploiting the new build_argv.
---
bootstrap.conf | 1 +
src/env.c | 385 ++---
2 files changed, 173 insertions(+), 213 deletions(-)
diff --git a/bootstrap.conf b/bootstrap.conf
index ab6b3ef0c..f55da99db 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -134,6 +134,7 @@ gnulib_modules="
host-os
human
idcache
+ idx
ignore-value
inttostr
inttypes
diff --git a/src/env.c b/src/env.c
index 11db374d9..e2ab39fd5 100644
--- a/src/env.c
+++ b/src/env.c
@@ -26,6 +26,8 @@
#include "system.h"
#include "die.h"
#include "error.h"
+#include "idx.h"
+#include "minmax.h"
#include "operand2sig.h"
#include "quote.h"
#include "sig2str.h"
@@ -41,14 +43,14 @@
/* Array of envvars to unset. */
static const char **usvars;
static size_t usvars_alloc;
-static ptrdiff_t usvars_used;
+static idx_t usvars_used;
/* Annotate the output with extra info to aid the user. */
static bool dev_debug;
/* Buffer and length of extracted envvars in -S strings. */
static char *varname;
-static ptrdiff_t vnlen;
+static idx_t vnlen;
/* Possible actions on each signal. */
enum SIGNAL_MODE {
@@ -175,7 +177,7 @@ append_unset_var (const char *var)
static void
unset_envvars (void)
{
- for (ptrdiff_t i = 0; i < usvars_used; ++i)
+ for (idx_t i = 0; i < usvars_used; ++i)
{
devmsg ("unset:%s\n", usvars[i]);
@@ -190,29 +192,6 @@ unset_envvars (void)
IF_LINT (usvars_alloc = 0);
}
-static bool _GL_ATTRIBUTE_PURE
-valid_escape_sequence (const char c)
-{
- return (c == 'c' || c == 'f' || c == 'n' || c == 'r' || c == 't' || c == 'v' \
- || c == '#' || c == '$' || c == '_' || c == '"' || c == '\'' \
- || c == '\\');
-}
-
-static char _GL_ATTRIBUTE_PURE
-escape_char (const char c)
-{
- switch (c)
-{
-/* \a, \b not supported by FreeBSD's env. */
-case 'f': return '\f';
-case 'n': return '\n';
-case 'r': return '\r';
-case 't': return '\t';
-case 'v': return '\v';
-default: assume (false);
-}
-}
-
/* Return a pointer to the end of a valid ${VARNAME} string, or NULL.
'str' should point to the '$' character.
First letter in VARNAME must be alpha or underscore,
@@ -241,7 +220,7 @@ scan_varname (const char *str)
static char *
extract_varname (const char *str)
{
- ptrdiff_t i;
+ idx_t i;
const char *p;
p = scan_varname (str);
@@ -263,150 +242,127 @@ extract_varname (const char *str)
return varname;
}
-/* Validate the "-S" parameter, according to the syntax defined by FreeBSD's
- env(1). Terminate with an error message if not valid.
+/* Temporary buffer used by --split-string processing. */
+struct splitbuf
+{
+ /* Buffer address, arg count, and half the number of elements in the buffer.
+ ARGC and ARGV are as in 'main', and ARGC + 1 <= HALF_ALLOC so
+ that the upper half of ARGV can be used for string contents.
+ This may waste up to half the space but keeps the code simple,
+ which is better for this rarely-used but security-sensitive code.
+
+ ARGV[0] is not initialized; that is the caller's responsibility
+ after finalization.
+
+ During assembly, ARGV[I] (where 0 < I < ARGC) contains the offset
+ of the Ith string (relative to ARGV + HALF_ALLOC), so that
+ reallocating ARGV does not change the validity of its contents.
+ The integer offset is cast to char * during assembly, and is
+ converted to a true char * pointer on finalization.
+
+ During assembly, ARGV[ARGC] contains the offset of the first
+ unused string byte (relative to ARGV + HALF_ALLOC). */
+ char **argv;
+ int argc;
+ idx_t half_alloc;
+
+ /* The number of extra argv slots to keep room for. */
+ int extra_argc;
+
+ /* Whether processing should act as if the most
