od: fix various off-by-one issues with --strings with -N

* src/od.c (dump_strings): There are three related issues here
due to not accounting for the terminating NUL char appropriately.

1. Ensure BUF always has enough space for the terminating NUL.
This avoids CWE-122: Heap-based Buffer Overflow,
where we wrote a single NUL byte directly after the allocated buffer.
I.e., there should be no buffer overflow with:
  printf '%100s' | od -N100 -S1

2. Ensure we support -S == -N (END_OFFSET - STRING_MIN == ADDRESS):
I.e., there should be output with:
  printf '%100s' | od -N10 -S10

3. Ensure we always output a valid address by ensuring
the ADDRESS and I variables are kept in sync.
I.e., this should output address 0000000 not 1777777777777777777777:
  printf '%100s' | od -N10 -S1

As well as fixing these we simplify by using a single loop
to read the data, rather than two.

* doc/coreutils.texi (od invocation): Clarify that -N
implicitly NUL terminates strings.
* tests/od/od-N.sh: Add test cases.
* NEWS: Mention the bug fixes.

Fixes https://bugs.gnu.org/78880

diff --git a/NEWS b/NEWS
index 8b3d9d3cc256483342811843e6ffc894a81d0b9e..e4d9c9b608f94fbeefff759f432b9e7d40129b80 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   copying to non-NFS files from NFSv4 files with trivial ACLs.
   [bug introduced in coreutils-9.6]
 
+  od --strings with -N now works correctly.  Previously od might
+  write a NUL byte after a heap buffer, or output invalid addresses.
+  [These bugs were present in "the beginning".]
+
   sort with key character offsets of SIZE_MAX, could induce
   a read of 1 byte before an allocated heap buffer. For example:
   'sort +0.18446744073709551615R input' on 64 bit systems.

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index a1d45fb301221bbb5b089ff8d64585218d94fb41..865ceff05098aa3ca171ce92070b6c7df7c56079 100644 (file)
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -2075,6 +2075,8 @@ least @var{bytes} consecutive printable characters,
 followed by a zero byte (ASCII NUL).
 Prefixes and suffixes on @var{bytes} are interpreted as for the
 @option{-j} option.
+If combined with the @option{-N} option, truncated strings
+are considered ASCII NUL terminated.
 
 If @var{bytes} is omitted with @option{--strings}, the default is 3.
 

diff --git a/src/od.c b/src/od.c
index 88d467c73c811da1bcefcdd8db06df1efdc921c5..32e8498960964f30844e2dfdb22939702cdcff4a 100644 (file)
--- a/src/od.c
+++ b/src/od.c
@@ -1505,70 +1505,58 @@ dump (void)
 }
 
 /* STRINGS mode.  Find each "string constant" in the input.
-   A string constant is a run of at least 'string_min' ASCII
-   graphic (or formatting) characters terminated by a null.
+   A string constant is a run of at least STRING_MIN
+   printable characters terminated by a NUL or END_OFFSET.
    Based on a function written by Richard Stallman for a
    traditional version of od.  Return true if successful.  */
 
 static bool
 dump_strings (void)
 {
-  idx_t bufsize = MAX (100, string_min);
+  idx_t bufsize = MAX (100, string_min + 1);
   char *buf = xmalloc (bufsize);
   uintmax_t address = n_bytes_to_skip;
   bool ok = true;
 
   while (true)
     {
-      idx_t i;
-      int c;
-
-      /* See if the next 'string_min' chars are all printing chars.  */
-    tryline:
+      idx_t i = 0;
+      int c = 1;  /* Init to 1 so can distinguish if NUL read.  */
 
       if (limit_bytes_to_format
-          && (end_offset < string_min || end_offset - string_min <= address))
+          && (end_offset < string_min || end_offset - string_min < address))
         break;
 
-      for (i = 0; i < string_min; i++)
-        {
-          ok &= read_char (&c);
-          address++;
-          if (c < 0)
-            {
-              free (buf);
-              return ok;
-            }
-          if (! isprint (c))
-            /* Found a non-printing.  Try again starting with next char.  */
-            goto tryline;
-          buf[i] = c;
-        }
-
-      /* We found a run of 'string_min' printable characters.
-         Now see if it is terminated with a null byte.  */
+      /* Store consecutive printable characters to BUF.  */
       while (!limit_bytes_to_format || address < end_offset)
         {
-          if (i == bufsize)
+          if (i == bufsize - 1)
             buf = xpalloc (buf, &bufsize, 1, -1, sizeof *buf);
           ok &= read_char (&c);
-          address++;
           if (c < 0)
             {
               free (buf);
               return ok;
             }
+          address++;
+          buf[i++] = c;
           if (c == '\0')
-            break;             /* It is; print this string.  */
+            break;             /* Print this string.  */
           if (! isprint (c))
-            goto tryline;      /* It isn't; give up on this string.  */
-          buf[i++] = c;                /* String continues; store it all.  */
+            {
+              c = -1;          /* Give up on this string.  */
+              break;
+            }
         }
 
-      /* If we get here, the string is all printable and null-terminated,
-         so print it.  It is all in 'buf' and 'i' is its length.  */
+      if (c == -1 || i - !c < string_min)
+        continue;
+
       buf[i] = 0;
-      format_address (address - i - 1, ' ');
+
+      /* If we get here, the string is all printable, so print it.  */
+
+      format_address (address - i, ' ');
 
       for (i = 0; (c = buf[i]); i++)
         {

diff --git a/tests/od/od-N.sh b/tests/od/od-N.sh
index b057c9b83b2a9edd42138f3e32ae9f5be56cff41..7c97eeee2a0d5260ab710d604bc35156aa3e4b04 100755 (executable)
--- a/tests/od/od-N.sh
+++ b/tests/od/od-N.sh
@@ -20,8 +20,6 @@
 print_ver_ od
 
 echo abcdefg > in || framework_failure_
-
-
 (od -An -N3 -c; od -An -N3 -c) < in > out
 cat <<EOF > exp || framework_failure_
    a   b   c
@@ -29,4 +27,31 @@ cat <<EOF > exp || framework_failure_
 EOF
 compare exp out || fail=1
 
+# coreutils <= 9.7 would buffer overflow with
+# a single NUL byte after the heap buffer
+printf '%100s' | od -N100 -S1 > out || fail=1
+printf '%07o %100s\n' 0 '' > exp || framework_failure_
+compare exp out || fail=1
+
+# coreutils <= 9.7 would output nothing
+printf '%100s' | od -N10 -S10 > out || fail=1
+printf '%07o %10s\n' 0 '' > exp || framework_failure_
+compare exp out || fail=1
+
+# coreutils <= 9.7 would output an invalid address
+printf '%100s' | od -N10 -S1 > out || fail=1
+printf '%07o %10s\n' 0 '' > exp || framework_failure_
+compare exp out || fail=1
+
+# Ensure -S limits appropriately
+printf '%10s\000' | od -N11 -S11 > out || fail=1
+compare /dev/null out || fail=1
+printf '%10s\000' | od -S11 > out || fail=1
+compare /dev/null out || fail=1
+printf '%10s' | od -S10 > out || fail=1 # Ignore unterminated at EOF?
+compare /dev/null out || fail=1
+printf '\001%10s\000%10s\000' | od -S10 > out || fail=1
+printf '%07o %10s\n' 1 '' 12 '' > exp || framework_failure_
+compare exp out || fail=1
+
 Exit $fail