While looking at GNU Make recently, I noticed an unlikely pointer
overflow, and also a couple of undesirable buffer truncations in the W32
code. Proposed patches attached. The first three patches are minor
cleanups. Patch 0004 fixes the pointer overflow, and patch 0005 fixes
the truncations.
It might make code easier to review if we got rid of the GET_PATH_MAX
and PATH_VAR macros and replaced uses with their definiens, since
GET_PATH_MAX is always PATH_MAX and PATH_VAR is a function-like macro
that cannot be implemented as a function.From c1ed5107b2391b5b09bc64dc93bbb085e7a7b7ea Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 5 Aug 2024 00:44:28 -0700
Subject: [PATCH 1/5] Avoid strlen calls after sprintf
* src/file.c (file_timestamp_sprintf):
* src/function.c (func_words, func_call):
* src/job.c (child_error):
* src/main.c (define_makeflags):
* src/output.c (message, error, fatal):
Use return value from sprintf instead of calling strlen
on the resulting buffer.
---
src/file.c | 14 ++++++--------
src/function.c | 9 +++------
src/job.c | 3 +--
src/main.c | 9 +++++----
src/output.c | 35 ++++++++++++++---------------------
5 files changed, 29 insertions(+), 41 deletions(-)
diff --git a/src/file.c b/src/file.c
index 77c328ca..3b1a8d31 100644
--- a/src/file.c
+++ b/src/file.c
@@ -1027,22 +1027,20 @@ file_timestamp_sprintf (char *p, FILE_TIMESTAMP ts)
if (tm)
{
intmax_t year = tm->tm_year;
- sprintf (p, "%04" PRIdMAX "-%02d-%02d %02d:%02d:%02d",
- year + 1900, tm->tm_mon + 1, tm->tm_mday,
- tm->tm_hour, tm->tm_min, tm->tm_sec);
+ p += sprintf (p, "%04" PRIdMAX "-%02d-%02d %02d:%02d:%02d",
+ year + 1900, tm->tm_mon + 1, tm->tm_mday,
+ tm->tm_hour, tm->tm_min, tm->tm_sec);
}
else if (t < 0)
- sprintf (p, "%" PRIdMAX, (intmax_t) t);
+ p += sprintf (p, "%" PRIdMAX, (intmax_t) t);
else
- sprintf (p, "%" PRIuMAX, (uintmax_t) t);
- p += strlen (p);
+ p += sprintf (p, "%" PRIuMAX, (uintmax_t) t);
/* Append nanoseconds as a fraction, but remove trailing zeros. We don't
know the actual timestamp resolution, since clock_getres applies only to
local times, whereas this timestamp might come from a remote filesystem.
So removing trailing zeros is the best guess that we can do. */
- sprintf (p, ".%09d", FILE_TIMESTAMP_NS (ts));
- p += strlen (p) - 1;
+ p += sprintf (p, ".%09d", FILE_TIMESTAMP_NS (ts)) - 1;
while (*p == '0')
p--;
p += *p != '.';
diff --git a/src/function.c b/src/function.c
index 82a9fe26..ee32373b 100644
--- a/src/function.c
+++ b/src/function.c
@@ -737,8 +737,7 @@ func_words (char *o, char **argv, const char *funcname UNUSED)
while (find_next_token (&word_iterator, NULL) != 0)
++i;
- sprintf (buf, "%u", i);
- o = variable_buffer_output (o, buf, strlen (buf));
+ o = variable_buffer_output (o, buf, sprintf (buf, "%u", i));
return o;
}
@@ -2659,8 +2658,7 @@ func_call (char *o, char **argv, const char *funcname UNUSED)
{
char num[INTSTR_LENGTH];
- sprintf (num, "%u", i);
- define_variable (num, strlen (num), *argv, o_automatic, 0);
+ define_variable (num, sprintf (num, "%u", i), *argv, o_automatic, 0);
}
/* If the number of arguments we have is < max_args, it means we're inside
@@ -2672,8 +2670,7 @@ func_call (char *o, char **argv, const char *funcname UNUSED)
{
char num[INTSTR_LENGTH];
- sprintf (num, "%u", i);
- define_variable (num, strlen (num), "", o_automatic, 0);
+ define_variable (num, sprintf (num, "%u", i), "", o_automatic, 0);
}
/* Expand the function in the context of the arguments, adding the result to
diff --git a/src/job.c b/src/job.c
index db741fc0..1c23cd73 100644
--- a/src/job.c
+++ b/src/job.c
@@ -557,9 +557,8 @@ child_error (struct child *child,
{
#define SHUFFLE_PREFIX " shuffle="
char *a = alloca (CSTRLEN(SHUFFLE_PREFIX) + strlen (smode) + 1);
- sprintf (a, SHUFFLE_PREFIX "%s", smode);
+ l += sprintf (a, SHUFFLE_PREFIX "%s", smode);
smode = a;
- l += strlen (smode);
#undef SHUFFLE_PREFIX
}
diff --git a/src/main.c b/src/main.c
index 0367f08b..e8d19382 100644
--- a/src/main.c
+++ b/src/main.c
@@ -3587,10 +3587,11 @@ define_makeflags (int makefile)
{
/* Add the value if not omitted. */
char *buf = alloca (30);
- sprintf (buf, "%u", *(unsigned int *) cs->value_ptr);
+ int buflen = sprintf (buf, "%u",
+ *(unsigned int *) cs->value_ptr);
if (!short_option (cs->c))
fp = variable_buffer_output (fp, "=", 1);
- fp = variable_buffer_output (fp, buf, strlen (buf));
+ fp = variable_buffer_output (fp, buf, buflen);
}
break;
@@ -3603,10 +3604,10 @@ define_makeflags (int makefile)
|| (*(double *) cs->value_ptr != *(double *) cs->noarg_value))
{
char *buf = alloca (100);
- sprintf (buf, "%g", *(double *) cs->value_ptr);
+ int buflen = sprintf (buf, "%g", *(double *) cs->value_ptr);
if (!short_option (cs->c))
fp = variable_buffer_output (fp, "=", 1);
- fp = variable_buffer_output (fp, buf, strlen (buf));
+ fp = variable_buffer_output (fp, buf, buflen);
}
break;
diff --git a/src/output.c b/src/output.c
index 775d1410..b7735ec4 100644
--- a/src/output.c
+++ b/src/output.c
@@ -425,13 +425,9 @@ message (int prefix, size_t len, const char *fmt, ...)
start = p = get_buffer (len);
if (prefix)
- {
- if (makelevel == 0)
- sprintf (p, "%s: ", program);
- else
- sprintf (p, "%s[%u]: ", program, makelevel);
- p += strlen (p);
- }
+ p += (makelevel == 0
+ ? sprintf (p, "%s: ", program)
+ : sprintf (p, "%s[%u]: ", program, makelevel));
va_start (args, fmt);
vsprintf (p, fmt, args);
@@ -457,13 +453,11 @@ error (const floc *flocp, size_t len, const char *fmt, ...)
+ INTSTR_LENGTH + 4 + 1 + 1);
start = p = get_buffer (len);
- if (flocp && flocp->filenm)
- sprintf (p, "%s:%lu: ", flocp->filenm, flocp->lineno + flocp->offset);
- else if (makelevel == 0)
- sprintf (p, "%s: ", program);
- else
- sprintf (p, "%s[%u]: ", program, makelevel);
- p += strlen (p);
+ p += (flocp && flocp->filenm
+ ? sprintf (p, "%s:%lu: ", flocp->filenm, flocp->lineno + flocp->offset)
+ : makelevel == 0
+ ? sprintf (p, "%s: ", program)
+ : sprintf (p, "%s[%u]: ", program, makelevel));
va_start (args, fmt);
vsprintf (p, fmt, args);
@@ -490,13 +484,12 @@ fatal (const floc *flocp, size_t len, const char *fmt, ...)
+ INTSTR_LENGTH + 8 + strlen (stop) + 1);
start = p = get_buffer (len);
- if (flocp && flocp->filenm)
- sprintf (p, "%s:%lu: *** ", flocp->filenm, flocp->lineno + flocp->offset);
- else if (makelevel == 0)
- sprintf (p, "%s: *** ", program);
- else
- sprintf (p, "%s[%u]: *** ", program, makelevel);
- p += strlen (p);
+ p += (flocp && flocp->filenm
+ ? sprintf (p, "%s:%lu: *** ", flocp->filenm,
+ flocp->lineno + flocp->offset)
+ : makelevel == 0
+ ? sprintf (p, "%s: *** ", program)
+ : sprintf (p, "%s[%u]: *** ", program, makelevel));
va_start (args, fmt);
vsprintf (p, fmt, args);
--
2.45.2
From 1793a0dd39864cd71cd5461947c369b9c9df24a6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 5 Aug 2024 01:04:13 -0700
Subject: [PATCH 2/5] Omit unused PATH_MAX code
* src/makeint.h (GET_PATH_MAX, PATH_VAR):
Simplify, since PATH_MAX is always defined here.
(NEED_GET_PATH_MAX): Remove.
* src/misc.c (get_path_max) [NEED_GET_PATH_MAX]: Remove.
---
src/makeint.h | 11 ++---------
src/misc.c | 19 -------------------
2 files changed, 2 insertions(+), 28 deletions(-)
diff --git a/src/makeint.h b/src/makeint.h
index 5a553093..61c78229 100644
--- a/src/makeint.h
+++ b/src/makeint.h
@@ -149,15 +149,8 @@ extern int errno;
# endif
#endif
-#ifdef PATH_MAX
-# define GET_PATH_MAX PATH_MAX
-# define PATH_VAR(var) char var[PATH_MAX+1]
-#else
-# define NEED_GET_PATH_MAX 1
-# define GET_PATH_MAX (get_path_max ())
-# define PATH_VAR(var) char *var = alloca (GET_PATH_MAX+1)
-unsigned int get_path_max (void);
-#endif
+#define GET_PATH_MAX PATH_MAX
+#define PATH_VAR(var) char var[PATH_MAX+1]
#ifndef CHAR_BIT
# define CHAR_BIT 8
diff --git a/src/misc.c b/src/misc.c
index 1914aa22..9e41a546 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -948,25 +948,6 @@ strncasecmp (const char *s1, const char *s2, size_t n)
#endif
-#ifdef NEED_GET_PATH_MAX
-unsigned int
-get_path_max (void)
-{
- static unsigned int value;
-
- if (value == 0)
- {
- long x = pathconf ("/", _PC_PATH_MAX);
- if (x > 0)
- value = (unsigned int) x;
- else
- value = PATH_MAX;
- }
-
- return value;
-}
-#endif
-
#if !HAVE_MEMPCPY
void *
mempcpy (void *dest, const void *src, size_t n)
--
2.45.2
From f58172ae143c974228d5a7861cb71b6a63693fa8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 5 Aug 2024 01:21:09 -0700
Subject: [PATCH 3/5] Prefer memcpy to strncpy if either will do
strncpy is trickier and a bit slower.
* src/function.c (func_realpath, func_abspath):
* src/misc.c (xstrndup):
Prefer memcpy or mempcpy to strncpy when the source length is known.
---
src/function.c | 12 ++++++------
src/misc.c | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/function.c b/src/function.c
index ee32373b..b45b8ee8 100644
--- a/src/function.c
+++ b/src/function.c
@@ -2109,7 +2109,7 @@ abspath (const char *name, char *apath)
apath[3] = '/';
dest++;
root_len++;
- /* strncpy above copied one character too many. */
+ /* memcpy above copied one character too many. */
name--;
}
else
@@ -2178,13 +2178,13 @@ func_realpath (char *o, char **argv, const char *funcname UNUSED)
{
if (len < GET_PATH_MAX)
{
- char *rp;
+ char *rp, *inend;
struct stat st;
PATH_VAR (in);
PATH_VAR (out);
- strncpy (in, path, len);
- in[len] = '\0';
+ inend = mempcpy (in, path, len);
+ *inend = '\0';
#ifdef HAVE_REALPATH
ENULLLOOP (rp, realpath (in, out));
@@ -2353,9 +2353,9 @@ func_abspath (char *o, char **argv, const char *funcname UNUSED)
{
PATH_VAR (in);
PATH_VAR (out);
+ char *inend = mempcpy (in, path, len);
- strncpy (in, path, len);
- in[len] = '\0';
+ *inend = '\0';
if (abspath (in, out))
{
diff --git a/src/misc.c b/src/misc.c
index 9e41a546..b36248f0 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -351,7 +351,7 @@ xstrndup (const char *str, size_t length)
#else
result = xmalloc (length + 1);
if (length > 0)
- strncpy (result, str, length);
+ memcpy (result, str, length);
result[length] = '\0';
#endif
--
2.45.2
From 3be9283627758e504589daae87d750fd6f995416 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 5 Aug 2024 01:30:44 -0700
Subject: [PATCH 4/5] Fix unlikely pointer overflow in abspath
* src/function.c (abspath): len is now ptrdiff_t,
to avoid GCC warning about comparing signed to unsigned.
It really is a pointer difference, after all.
Rejigger comparision to avoid undefined behavior
if dest + len is an invalid pointer.
---
src/function.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/function.c b/src/function.c
index b45b8ee8..12b1456a 100644
--- a/src/function.c
+++ b/src/function.c
@@ -2119,7 +2119,7 @@ abspath (const char *name, char *apath)
for (start = end = name; *start != '\0'; start = end)
{
- size_t len;
+ ptrdiff_t len;
/* Skip sequence of multiple path-separators. */
while (ISDIRSEP (*start))
@@ -2147,7 +2147,7 @@ abspath (const char *name, char *apath)
if (! ISDIRSEP (dest[-1]))
*dest++ = '/';
- if (dest + len >= apath_limit)
+ if (apath_limit - dest <= len)
return NULL;
dest = mempcpy (dest, start, len);
--
2.45.2
From f20ffc2812bc0c092895918dbb2c1e88e823c1aa Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 5 Aug 2024 01:39:18 -0700
Subject: [PATCH 5/5] Check for snprintf truncation on W32
* src/main.c (find_and_set_default_shell) [MK_OS_W32]:
Do not use a buffer that snprintf truncated.
---
src/main.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/main.c b/src/main.c
index e8d19382..78084d09 100644
--- a/src/main.c
+++ b/src/main.c
@@ -1080,12 +1080,15 @@ find_and_set_default_shell (const char *token)
while (ep && *ep)
{
+ int sh_pathlen;
PATH_VAR (sh_path);
*ep = '\0';
- snprintf (sh_path, GET_PATH_MAX, "%s/%s", p, search_token);
- if (_access (sh_path, 0) == 0)
+ sh_pathlen = snprintf (sh_path, GET_PATH_MAX, "%s/%s",
+ p, search_token);
+ if (0 <= sh_pathlen && sh_pathlen < GET_PATH_MAX
+ && _access (sh_path, 0) == 0)
{
default_shell = xstrdup (w32ify (sh_path, 0));
sh_found = 1;
@@ -1106,9 +1109,13 @@ find_and_set_default_shell (const char *token)
/* be sure to check last element of Path */
if (p && *p)
{
+ int sh_pathlen;
+
PATH_VAR (sh_path);
- snprintf (sh_path, GET_PATH_MAX, "%s/%s", p, search_token);
- if (_access (sh_path, 0) == 0)
+ sh_pathlen = snprintf (sh_path, GET_PATH_MAX, "%s/%s",
+ p, search_token);
+ if (0 <= sh_pathlen && sh_pathlen < GET_PATH_MAX
+ && _access (sh_path, 0) == 0)
{
default_shell = xstrdup (w32ify (sh_path, 0));
sh_found = 1;
--
2.45.2
