GNU bug report logs - #78880
od Heap-buffer overflow

Previous Next

To reply to this bug, email your comments to 78880 AT debbugs.gnu.org.
There is no need to reopen the bug first.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jaehoon Jang <jaehoon.jang <at> prosys.kaist.ac.kr>
To: bug-coreutils <at> gnu.org
Subject: od Heap-buffer overflow
Date: Mon, 23 Jun 2025 17:24:53 +0900

[Message part 1 (text/plain, inline)]

*###### Build options*```
git clone https://github.com/coreutils/coreutils
export GNULIB_SRCDIR=./gnulib
export FORCE_UNSAFE_CONFIGURE=1
./bootstrap
CC="clang -g -fsanitize=address" CXX="clang -g -fsanitize=address"
./configure $CONFIG_OPTIONS
make -j
```



*###### od version*```
$ src/od --version
od (GNU coreutils) 9.7.52-b7db77
Copyright (C) 2025 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <
https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Jim Meyering.
```


*###### reproduce script*```c
// gcc -o replay_od replay_od.c

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#define MAX_CMDLINE_LEN 100000
#define MAX_CMDLINE_PAR 1000

int main() {
    static char in_buf[MAX_CMDLINE_LEN];
    static char *argv[MAX_CMDLINE_PAR];

    char *ptr = in_buf;
    int rc = 0;

    int n = read(0, in_buf, MAX_CMDLINE_LEN - 2);
    if (n <= 0) {
        perror("read");
        exit(1);
    }

    while (*ptr) {
        argv[rc] = ptr;

        if (argv[rc][0] == 0x02 && !argv[rc][1]) {
            argv[rc]++;
        }

        rc++;

        while (*ptr) ptr++;
        ptr++;
    }
    argv[0] = "od";

    // exec od with parsed argv
    execvp("src/od", argv);

    perror("execvp failed");
    return 1;
}
```
This is a C wrapper program that automatically sets argv to reproduce the
vulnerability. It is a simple wrapper around the od program and does not
modify the original od binary.

###### ASAN log
```
$ ./replay_od < /root/250623/od-poc
od: 'u'$'\353''d'$'\001''4
'\'''$'\350\003\346\346\346\346\027\027\027\027\027\027\027\027''!'$'\027\027\027\027\027\027\027\027\027\027\027\027\027\027\027\027''{{{{{{{{{{{{{{{{{{{{{{{{{{{{{{{{{{{'$'\024''{{{{'$'\200\016\377\377''{{{{{{{{{{{s{{'$'\220''{{{{'$'\213\207\213\213\213\213\213\213\213\213\213\213\213\213''}'$'\213\213\213\213\213\213\213\213\213\213''E':
No such file or directory
=================================================================
==1151699==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x6150000004f9 at pc 0x0000004d153f bp 0x7fff937f0410 sp 0x7fff937f0408
WRITE of size 1 at 0x6150000004f9 thread T0
    #0 0x4d153e in dump_strings coreutils/src/od.c:1570:14
    #1 0x4d153e in main coreutils/src/od.c:2037:30
    #2 0x7fc132ca6d8f in __libc_start_call_main
csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #3 0x7fc132ca6e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #4 0x41f664 in _start (coreutils/src/od+0x41f664)

0x6150000004f9 is located 0 bytes to the right of 505-byte region
[0x615000000300,0x6150000004f9)
allocated by thread T0 here:
    #0 0x49c843 in __interceptor_realloc (coreutils/src/od+0x49c843)
    #1 0x4ddcea in rpl_realloc coreutils/./lib/stdlib.h:2095:10
    #2 0x4ddcea in xrealloc coreutils/lib/xmalloc.c:66:13
    #3 0x4ddcea in xpalloc coreutils/lib/xmalloc.c:271:8
    #4 0x7fc132ca6d8f in __libc_start_call_main
csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: heap-buffer-overflow coreutils/src/od.c:1570:14
in dump_strings
Shadow bytes around the buggy address:
  0x0c2a7fff8040: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c2a7fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a7fff8060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a7fff8070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a7fff8080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c2a7fff8090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00[01]
  0x0c2a7fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a7fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a7fff80c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a7fff80d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a7fff80e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==1151699==ABORTING
```

*###### Description*
```
1513 static bool
1514 dump_strings (void)
1515 {
1516   idx_t bufsize = MAX (100, string_min);
1517   char *buf = xmalloc (bufsize);
...
1550       while (!limit_bytes_to_format || address < end_offset)
1551         {
1552           if (i == bufsize)
1553             buf = xpalloc (buf, &bufsize, 1, -1, sizeof *buf);
1554           ok &= read_char (&c);
1555           address++;
1556           if (c < 0)
1557             {
1558               free (buf);
1559               return ok;
1560             }
1561           if (c == '\0')
1562             break;              /* It is; print this string.  */
1563           if (! isprint (c))
1564             goto tryline;       /* It isn't; give up on this string.
 */
1565           buf[i++] = c;         /* String continues; store it all.  */
1566         }
...
1570  buf[i] = 0;
1571  format_address (address - i - 1, ' ');
```
The vulnerability occurs in the dump_strings function. The variable buf is
allocated with a size of bufsize, which is limited to a maximum of 100
bytes.

However, inside the while loop, the variable i is incremented by 1 on each
iteration. When the attached PoC file is passed as arguments to the od
program, the value of i can exceed 100. This leads to a heap buffer
overflow vulnerability.

An attacker could potentially exploit this by manipulating both the i and c
values, resulting in dangerous modifications to the buf chunk on the heap.

Therefore, we recommend adding proper bounds checking for the i variable to
prevent this issue.

[Message part 2 (text/html, inline)]

[od-poc (application/octet-stream, attachment)]

[replay_od.c (application/octet-stream, attachment)]

Message #8 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jaehoon Jang <jaehoon.jang <at> prosys.kaist.ac.kr>, 78880 <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Tue, 24 Jun 2025 01:16:44 +0100

On 23/06/2025 09:24, Jaehoon Jang wrote:
> =================================================================
> ==1151699==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x6150000004f9 at pc 0x0000004d153f bp 0x7fff937f0410 sp 0x7fff937f0408
> WRITE of size 1 at 0x6150000004f9 thread T0
>      #0 0x4d153e in dump_strings coreutils/src/od.c:1570:14

Nice fuzzing.

There looks to be all sorts of off by one errors in the dump_strings() function.
The issue is most easily demonstrated with:

  printf '%100s' | tr ' ' . | valgrind od -N100 -S99

The following should fix this I think.
I've only analyzed it for a few minutes, so I'll look more tomorrow.
The following should also fix the printed offset,
and also support the -N100 -S100 combination.

thanks!

Pádraig.

diff --git a/src/od.c b/src/od.c
index 88d467c73..7f0cd5ab7 100644
--- a/src/od.c
+++ b/src/od.c
@@ -1513,7 +1513,7 @@ dump (void)
 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;
@@ -1527,7 +1527,7 @@ dump_strings (void)
     tryline:

       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++)
@@ -1549,7 +1549,7 @@ dump_strings (void)
          Now see if it is terminated with a null byte.  */
       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++;
@@ -1568,7 +1568,7 @@ dump_strings (void)
       /* 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.  */
       buf[i] = 0;
-      format_address (address - i - 1, ' ');
+      format_address (address - i, ' ');

       for (i = 0; (c = buf[i]); i++)
         {

Message #13 received at 78880-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jaehoon Jang <jaehoon.jang <at> prosys.kaist.ac.kr>, 78880-done <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Tue, 24 Jun 2025 15:03:41 +0100

[Message part 1 (text/plain, inline)]

On 24/06/2025 01:16, Pádraig Brady wrote:
> On 23/06/2025 09:24, Jaehoon Jang wrote:
>> =================================================================
>> ==1151699==ERROR: AddressSanitizer: heap-buffer-overflow on address
>> 0x6150000004f9 at pc 0x0000004d153f bp 0x7fff937f0410 sp 0x7fff937f0408
>> WRITE of size 1 at 0x6150000004f9 thread T0
>>       #0 0x4d153e in dump_strings coreutils/src/od.c:1570:14
> 
> Nice fuzzing.
> 
> There looks to be all sorts of off by one errors in the dump_strings() function.
> The issue is most easily demonstrated with:
> 
>     printf '%100s' | tr ' ' . | valgrind od -N100 -S99
> 
> The following should fix this I think.
> I've only analyzed it for a few minutes, so I'll look more tomorrow.
> The following should also fix the printed offset,
> and also support the -N100 -S100 combination.

The previous patch didn't handle the invalid address output in all cases.
Also I didn't see a need for both read() loops in this function,
so I simplified the function in the attached more complete patch.

Marking this as done.

I'll apply this later.

thanks again,
Padraig.

[0001-od-fix-various-off-by-one-issues-with-strings-with-N.patch (text/x-patch, attachment)]

Message #16 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Grisha Levit <grishalevit <at> gmail.com>
To: 78880 <at> debbugs.gnu.org, P <at> draigbrady.com, jaehoon.jang <at> prosys.kaist.ac.kr
Cc: 78880-done <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Tue, 24 Jun 2025 10:27:36 -0400

On Tue, Jun 24, 2025, 10:04 Pádraig Brady <P <at> draigbrady.com> wrote:
>
> On 24/06/2025 01:16, Pádraig Brady wrote:
> > On 23/06/2025 09:24, Jaehoon Jang wrote:
> >> =================================================================
> >> ==1151699==ERROR: AddressSanitizer: heap-buffer-overflow on address
> >> 0x6150000004f9 at pc 0x0000004d153f bp 0x7fff937f0410 sp 0x7fff937f0408
> >> WRITE of size 1 at 0x6150000004f9 thread T0
> >>       #0 0x4d153e in dump_strings coreutils/src/od.c:1570:14
> >
> > Nice fuzzing.
> >
> > There looks to be all sorts of off by one errors in the dump_strings() function.
> > The issue is most easily demonstrated with:
> >
> >     printf '%100s' | tr ' ' . | valgrind od -N100 -S99
> >
> > The following should fix this I think.
> > I've only analyzed it for a few minutes, so I'll look more tomorrow.
> > The following should also fix the printed offset,
> > and also support the -N100 -S100 combination.
>
> The previous patch didn't handle the invalid address output in all cases.
> Also I didn't see a need for both read() loops in this function,
> so I simplified the function in the attached more complete patch.

This part:

>        tryline:
>    +     idx_t i = 0;
>    +     int c = 1; /* Init to 1 so can distinguish if NUL read. */

triggers

    warning: label followed by a declaration is a C23 extension
[-Wc23-extensions]

Message #22 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Grisha Levit <grishalevit <at> gmail.com>, 78880 <at> debbugs.gnu.org,
 jaehoon.jang <at> prosys.kaist.ac.kr
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Tue, 24 Jun 2025 16:27:46 +0100

[Message part 1 (text/plain, inline)]

On 24/06/2025 15:27, Grisha Levit wrote:

> This part:
> 
>>         tryline:
>>     +     idx_t i = 0;
>>     +     int c = 1; /* Init to 1 so can distinguish if NUL read. */
> 
> triggers
> 
>      warning: label followed by a declaration is a C23 extension
> [-Wc23-extensions]

OK version 3 attached which simplifies
by removing the label and gotos entirely.

thanks!

Padraig

[0001-od-fix-various-off-by-one-issues-with-strings-with-N.patch (text/x-patch, attachment)]

Message #25 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Collin Funk <collin.funk1 <at> gmail.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 78880 <at> debbugs.gnu.org, jaehoon.jang <at> prosys.kaist.ac.kr,
 Grisha Levit <grishalevit <at> gmail.com>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Tue, 24 Jun 2025 09:34:05 -0700

Pádraig Brady <P <at> draigBrady.com> writes:

> On 24/06/2025 15:27, Grisha Levit wrote:
>
>> This part:
>> 
>>>         tryline:
>>>     +     idx_t i = 0;
>>>     +     int c = 1; /* Init to 1 so can distinguish if NUL read. */
>> triggers
>>      warning: label followed by a declaration is a C23 extension
>> [-Wc23-extensions]
>
> OK version 3 attached which simplifies
> by removing the label and gotos entirely.

Grisha, what did you pass to ./configure?

I'm suprised we don't disable this in Gnulib. Since there we do:

    static_assert (1 < sizeof (example))

Without a second argument message string, I would expect Clang to
complain about it, unless we disable it.

Collin

Message #28 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 78880 <at> debbugs.gnu.org, jaehoon.jang <at> prosys.kaist.ac.kr,
 Grisha Levit <grishalevit <at> gmail.com>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Tue, 24 Jun 2025 18:31:14 -0700

On Tue, Jun 24, 2025 at 8:29 AM Pádraig Brady <P <at> draigbrady.com> wrote:
> OK version 3 attached which simplifies
> by removing the label and gotos entirely.

Thanks for fixing my very old bugs! That goes way back. I think od.c
was the second stand-alone program I contributed to coreutils (first
was tr). The earliest email I still have that mentions it is from
1997-01 prior to textutils-1.22, but that was just a ChangeLog entry
about adapting to a changed strtod API.

Message #31 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Grisha Levit <grishalevit <at> gmail.com>
To: Collin Funk <collin.funk1 <at> gmail.com>
Cc: 78880 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Fri, 27 Jun 2025 12:19:12 -0400

On Tue, Jun 24, 2025, 12:34 Collin Funk <collin.funk1 <at> gmail.com> wrote:
>
> Pádraig Brady <P <at> draigBrady.com> writes:
>
> > On 24/06/2025 15:27, Grisha Levit wrote:
> >
> >> This part:
> >>
> >>>         tryline:
> >>>     +     idx_t i = 0;
> >>>     +     int c = 1; /* Init to 1 so can distinguish if NUL read. */
> >> triggers
> >>      warning: label followed by a declaration is a C23 extension
> >> [-Wc23-extensions]
> >
> > OK version 3 attached which simplifies
> > by removing the label and gotos entirely.
>
> Grisha, what did you pass to ./configure?
>
> I'm suprised we don't disable this in Gnulib. Since there we do:
>
>     static_assert (1 < sizeof (example))
>
> Without a second argument message string, I would expect Clang to
> complain about it, unless we disable it.

Sorry, TBH I didn't do a proper build, I just remembered the
discussion at [1] while reading the patch.

[1] https://lists.gnu.org/r/bug-gnulib/2024-04/msg00044.html

Message #34 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Collin Funk <collin.funk1 <at> gmail.com>
To: Grisha Levit <grishalevit <at> gmail.com>
Cc: 78880 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Fri, 27 Jun 2025 09:45:03 -0700

Hi Grisha,

Grisha Levit <grishalevit <at> gmail.com> writes:

>> Grisha, what did you pass to ./configure?
>>
>> I'm suprised we don't disable this in Gnulib. Since there we do:
>>
>>     static_assert (1 < sizeof (example))
>>
>> Without a second argument message string, I would expect Clang to
>> complain about it, unless we disable it.
>
> Sorry, TBH I didn't do a proper build, I just remembered the
> discussion at [1] while reading the patch.
>
> [1] https://lists.gnu.org/r/bug-gnulib/2024-04/msg00044.html

No problem at all, I was just curious.

Gnulib can add support for C23's single argument static_assert if the
compiler does not support it, but cannot make it support declarations
after labels. So good catch on your part.

Thanks!
Collin

Message #37 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 78880 <at> debbugs.gnu.org, jaehoon.jang <at> prosys.kaist.ac.kr,
 Pádraig Brady <P <at> draigbrady.com>,
 Grisha Levit <grishalevit <at> gmail.com>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sat, 28 Jun 2025 21:25:12 -0700

[Message part 1 (text/plain, inline)]

On 2025-06-24 18:31, Jim Meyering wrote:
> That goes way back. I think od.c
> was the second stand-alone program I contributed to coreutils (first
> was tr). The earliest email I still have that mentions it is from
> 1997-01 prior to textutils-1.22, but that was just a ChangeLog entry
> about adapting to a changed strtod API.

I have a soft spot for 'od' as I remember using it in Unix in the 1970s. 
So I looked for nearby bugs and found a few, mostly integer overflows. I 
installed the attached patches to refactor the source and to fix the 
bugs I found.

You might be amused by patch 0007, which fixes a POSIX conformance bug 
introduced in January 1995, in what is now Git commit 
851162a0da41f2b6b08a8c1ed045086db9a443a0. Evidently this POSIX-required 
feature is not often used! The NEWS item in the fix says "[bug 
introduced on 1995-01-25]" instead of the usual "[bug introduced in 
coreutils-N]" comment because I don't know how to relate that commit to 
a version number (would it be textutils? probably doesn't matter).

You might also be amused (or appalled) by patch 0007's hacky fix. I 
couldn't bestir myself to write a cleaner fix. The hacky fix doesn't 
require memory allocation so in some sense it's better than a cleaner 
one would be.

[0001-od-fix-theoretical-size_t-malloc-overflow.patch (text/x-patch, attachment)]

[0002-od-fix-another-off-by-one-issue-with-strings.patch (text/x-patch, attachment)]

[0003-maint-assume-long-long-int.patch (text/x-patch, attachment)]

[0004-od-don-t-assume-no-holes-in-wide-unsigned.patch (text/x-patch, attachment)]

[0005-od-prefer-idx_t-to-size_t.patch (text/x-patch, attachment)]

[0006-od-fix-some-unlikely-integer-overflows.patch (text/x-patch, attachment)]

[0007-od-fix-N.-bug.patch (text/x-patch, attachment)]

[0008-od-prefer-intmax_t-to-uintmax_t.patch (text/x-patch, attachment)]

[0009-od-initialize-type-size-tables-statically.patch (text/x-patch, attachment)]

[0010-od-support-uintmax_t-too.patch (text/x-patch, attachment)]

[0011-od-replace-lookup-tables-with-simple-arithmetic.patch (text/x-patch, attachment)]

[0012-od-omit-some-duplicate-code.patch (text/x-patch, attachment)]

[0013-od-minor-lcm-tuning.patch (text/x-patch, attachment)]

[0014-od-simpler-static-initialization.patch (text/x-patch, attachment)]

[0015-od-simplify-away-one-loop-copy.patch (text/x-patch, attachment)]

[0016-od-check-sign-bit-more-often.patch (text/x-patch, attachment)]

[0017-od-speed-up-S.patch (text/x-patch, attachment)]

[0018-od-fix-integer-overflow-with-large-pseudos.patch (text/x-patch, attachment)]

[0019-od-be-more-consistent-re-sizeof.patch (text/x-patch, attachment)]

Message #40 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Collin Funk <collin.funk1 <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 78880 <at> debbugs.gnu.org, jaehoon.jang <at> prosys.kaist.ac.kr,
 Pádraig Brady <P <at> draigbrady.com>,
 Jim Meyering <jim <at> meyering.net>, Grisha Levit <grishalevit <at> gmail.com>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sat, 28 Jun 2025 22:03:25 -0700

Hi Paul,

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> * src/od.c (width_bytes, decode_one_format): Don’t assume a signed
> type has the same size as the corresponding unsigned type.
> This has no effect on practical platforms; it’s just for
> consistency there.
> ---
>  src/od.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/src/od.c b/src/od.c
> index c3c76cc86..50319aa83 100644
> --- a/src/od.c
> +++ b/src/od.c
> @@ -131,15 +131,15 @@ struct tspec
>  static const int width_bytes[] =
>  {
>    -1,
> -  sizeof (char),
> +  sizeof (unsigned char),
>  #if UCHAR_MAX < USHRT_MAX
> -  sizeof (short int),
> +  sizeof (unsigned short int),

I haven't looked at all the patches (started backwards). But regarding
the commit message, the assumption is safe if we assume ISO C
conformance. C99 states in § 6.2.5:

    For each of the signed integer types, there is a corresponding (but
    different) unsigned integer type (designated with the keyword
    unsigned) that uses the same amount of storage (including sign
    information) and has the same alignment requirements.

C23 has similar wording for the *_WIDTH macros that it added. For
U?INT_WIDTH for example, from § 5.2.5.3.2:

    The macro INT_WIDTH represents the width of the type int and shall
    expand to the same value as UINT_WIDTH.

The patch is fine, and looks a bit cleaner when matching the U*_MAX in
preprocessor directives. But figured I should let you know about those
sections of the C standard if you did not know them already.

Collin

Message #43 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 78880 <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sun, 29 Jun 2025 13:17:34 +0100

On 29/06/2025 05:25, Paul Eggert wrote:

> od: omit some duplicate code
> On x86-64 (for example) print_long, print_long_long, and
> print_intmax all behave identically, so give GCC enough info so
> that it generates code for just one of these functions.
> * src/od.c (enum size_spec): Arrange for enum values to
> be the same if they represent types that behave the same.
> (width_bytes, ISPEC_TO_FORMAT, decode_one_format):
> Match the enum size_spec changes.

The patch above causes a warning on some systems:

  src/od.c: In function 'decode_one_format':
  src/od.c:900:28: error: duplicated 'if' condition [-Werror=duplicated-cond]
    900 |         else if (size_spec == FLOAT_HALF)
        |                  ~~~~~~~~~~^~~~~~~~~~~~~
  src/od.c:895:28: note: previously used here
    895 |         else if (size_spec == FLOAT_SINGLE)
        |                  ~~~~~~~~~~^~~~~~~~~~~~~~~

If we want the compiler to just apply Dead Code Elimination here,
then it may be best to push/pop ignoring that warning ?

cheers,
Padraig

Message #46 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 78880 <at> debbugs.gnu.org, jaehoon.jang <at> prosys.kaist.ac.kr,
 Pádraig Brady <P <at> draigbrady.com>,
 Grisha Levit <grishalevit <at> gmail.com>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sun, 29 Jun 2025 09:21:39 -0700

On Sat, Jun 28, 2025 at 9:25 PM Paul Eggert <eggert <at> cs.ucla.edu> wrote:
> On 2025-06-24 18:31, Jim Meyering wrote:
> > That goes way back. I think od.c
> > was the second stand-alone program I contributed to coreutils (first
> > was tr). The earliest email I still have that mentions it is from
> > 1997-01 prior to textutils-1.22, but that was just a ChangeLog entry
> > about adapting to a changed strtod API.
>
> I have a soft spot for 'od' as I remember using it in Unix in the 1970s.
> So I looked for nearby bugs and found a few, mostly integer overflows. I
> installed the attached patches to refactor the source and to fix the
> bugs I found.
>
> You might be amused by patch 0007, which fixes a POSIX conformance bug
> introduced in January 1995, in what is now Git commit
> 851162a0da41f2b6b08a8c1ed045086db9a443a0. Evidently this POSIX-required
> feature is not often used! The NEWS item in the fix says "[bug
> introduced on 1995-01-25]" instead of the usual "[bug introduced in
> coreutils-N]" comment because I don't know how to relate that commit to
> a version number (would it be textutils? probably doesn't matter).
>
> You might also be amused (or appalled) by patch 0007's hacky fix. I
> couldn't bestir myself to write a cleaner fix. The hacky fix doesn't
> require memory allocation so in some sense it's better than a cleaner
> one would be.

Hi Paul,

Wow. Thanks for all of those fixes (haven't looked at all of them yet).
As for how to determine which release has the fix from your 0007 patch,
that commit was after textutils-1.8b, and the following release was
textutils-2.0:

$ git desc 851162a0da41f2b6b08a8c1ed045086db9a443a0
TEXTUTILS-1_8b-499-g851162a0d
$ git tag |grep -A1 1.8b
TEXTUTILS-1_8b
TEXTUTILS-2_0

I'm not appalled :-)
It feels like accommodating the old format sort of justifies the hackiness.

Message #49 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 78880 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sun, 29 Jun 2025 09:30:14 -0700

On Sun, Jun 29, 2025 at 5:18 AM Pádraig Brady <P <at> draigbrady.com> wrote:
> On 29/06/2025 05:25, Paul Eggert wrote:
>
>  > od: omit some duplicate code
>  > On x86-64 (for example) print_long, print_long_long, and
>  > print_intmax all behave identically, so give GCC enough info so
>  > that it generates code for just one of these functions.
>  > * src/od.c (enum size_spec): Arrange for enum values to
>  > be the same if they represent types that behave the same.
>  > (width_bytes, ISPEC_TO_FORMAT, decode_one_format):
>  > Match the enum size_spec changes.
>
> The patch above causes a warning on some systems:
>
>    src/od.c: In function 'decode_one_format':
>    src/od.c:900:28: error: duplicated 'if' condition [-Werror=duplicated-cond]
>      900 |         else if (size_spec == FLOAT_HALF)
>          |                  ~~~~~~~~~~^~~~~~~~~~~~~
>    src/od.c:895:28: note: previously used here
>      895 |         else if (size_spec == FLOAT_SINGLE)
>          |                  ~~~~~~~~~~^~~~~~~~~~~~~~~
>
> If we want the compiler to just apply Dead Code Elimination here,
> then it may be best to push/pop ignoring that warning ?

Thanks. I agree.

Message #52 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 78880 <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sun, 29 Jun 2025 20:59:14 +0100

On 29/06/2025 13:17, Pádraig Brady wrote:
> On 29/06/2025 05:25, Paul Eggert wrote:
> 
>   > od: omit some duplicate code
>   > On x86-64 (for example) print_long, print_long_long, and
>   > print_intmax all behave identically, so give GCC enough info so
>   > that it generates code for just one of these functions.
>   > * src/od.c (enum size_spec): Arrange for enum values to
>   > be the same if they represent types that behave the same.
>   > (width_bytes, ISPEC_TO_FORMAT, decode_one_format):
>   > Match the enum size_spec changes.
> 
> The patch above causes a warning on some systems:
> 
>     src/od.c: In function 'decode_one_format':
>     src/od.c:900:28: error: duplicated 'if' condition [-Werror=duplicated-cond]
>       900 |         else if (size_spec == FLOAT_HALF)
>           |                  ~~~~~~~~~~^~~~~~~~~~~~~
>     src/od.c:895:28: note: previously used here
>       895 |         else if (size_spec == FLOAT_SINGLE)
>           |                  ~~~~~~~~~~^~~~~~~~~~~~~~~
> 
> If we want the compiler to just apply Dead Code Elimination here,
> then it may be best to push/pop ignoring that warning ?

FYI, related to this I see our coverity instance is complaining about deadcode
in the following block, which might be suppressed with this change:

diff --git a/src/od.c b/src/od.cindex 50319aa83..c621ef227 100644
--- a/src/od.c
+++ b/src/od.c
@@ -802,12 +802,17 @@ decode_one_format (char const *s_orig, char const *s, char const **next,
          and prefer INTMAX to LONG_LONG.  */
       print_function
         = (size_spec == INT ? print_int
+           /* coverity[DEADCODE] */
            : size_spec == SHORT ? (fmt == SIGNED_DECIMAL
                                    ? print_s_short : print_short)
+           /* coverity[DEADCODE] */
            : size_spec == CHAR ? (fmt == SIGNED_DECIMAL
                                   ? print_s_char : print_char)
+           /* coverity[DEADCODE] */
            : size_spec == LONG ? print_long
+           /* coverity[DEADCODE] */
            : size_spec == INTMAX ? print_intmax
+           /* coverity[DEADCODE] */
            : size_spec == LONG_LONG ? print_long_long
            : (affirm (false), nullptr));
       break;


That's a bit messy to me though (the existing comment should
suffice for humans to know the deadcode is intentional).
So I've manually suppressed that error instance in our coverity instance.

cheers,
Padraig

Message #55 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 78880 <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sun, 29 Jun 2025 17:21:53 -0700

[Message part 1 (text/plain, inline)]

On 2025-06-29 05:17, Pádraig Brady wrote:
> If we want the compiler to just apply Dead Code Elimination here,
> then it may be best to push/pop ignoring that warning ?

Or, since GCC didn't complain about similar code elsewhere, we can 
change the complained-about code to look more like the code elsewhere. 
This is more likely to pacify any non-GCC compilers that are also flummoxed.

There's a similar situation with the floating point formats, which I 
assume you didn't run into because they're all distinct on your platform.

I installed the attached patch which I hope pacifies GCC on your 
platform. I didn't observe the problem with gcc (GCC) 15.1.1 20250521 
(Red Hat 15.1.1-2) on x86-64 so I can't easily test the patch myself.

[0001-od-pacify-gcc-Wduplicated-cond.patch (text/x-patch, attachment)]

Message #58 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 78880 <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sun, 29 Jun 2025 17:25:50 -0700

On 2025-06-29 12:59, Pádraig Brady wrote:
> I've manually suppressed that error instance in our coverity instance.

Maybe the change I just installed removed the need for that manual 
suppression?

Message #61 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 78880 <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Sun, 29 Jun 2025 22:34:17 -0700

[Message part 1 (text/plain, inline)]

As sometimes happens (in my case, while walking the dog) I thought of 
one or two more little problems in that area, and installed the attached 
patches to fix them. The first patch merely refactors; the second one 
does the fix; the third one adds test cases.

[0001-od-refactor-parse_old_offset.patch (text/x-patch, attachment)]

[0002-od-more-minor-fixes-for-offsets.patch (text/x-patch, attachment)]

[0003-od-add-test-cases-for-recent-fix.patch (text/x-patch, attachment)]

Message #64 received at 78880 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 78880 <at> debbugs.gnu.org
Subject: Re: bug#78880: od Heap-buffer overflow
Date: Mon, 30 Jun 2025 14:42:36 +0100

On 30/06/2025 01:25, Paul Eggert wrote:
> On 2025-06-29 12:59, Pádraig Brady wrote:
>> I've manually suppressed that error instance in our coverity instance.
> 
> Maybe the change I just installed removed the need for that manual
> suppression?

Indeed it does.
I checked with ./configure utils_cv_ieee_16_bit_supported=no
and also installed the following to avoid issues with that configuration:

I also installed a syntax-check tweak to use quotef() rather than quotearg_colon().

thanks!
Padraig

commit 87dcd447b165fee248ead5d15a8d2ea3eeaed15e (HEAD -> master)
Author: Pádraig Brady <P <at> draigBrady.com>
Date:   Mon Jun 30 14:25:56 2025 +0100

    od: reinstate half float validation check

    Reinstate check removed in commit 56aa549a0 so that we
    disallow -f2 when configured with utils_cv_ieee_16_bit_supported=no.
    Otherwise the output routines will consume floats,
    i.e. 4 bytes at a time.  Without this extra check
    the tests/od/od-endian.sh will fail with this configuration.

    * src/od.c (decode_one_format): Reinstate the explicit check
    for this configuration edge case.

diff --git a/src/od.c b/src/od.c
index 700470b85..24e981cb0 100644
--- a/src/od.c
+++ b/src/od.c
@@ -871,7 +871,9 @@ decode_one_format (char const *s_orig, char const *s, char const **next,
           else
             {
               if (ARRAY_CARDINALITY (fp_type_size) <= size
-                  || fp_type_size[size] == NO_SIZE)
+                  || fp_type_size[size] == NO_SIZE
+                  || (! FLOAT16_SUPPORTED && BF16_SUPPORTED
+                      && size == sizeof (bfloat16)))
                 {
                   error (0, 0,
                          _("invalid type string %s;\n"

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.