GNU bug report logs - #28120
[PATCH] ptx: fix a possible crash caused by integer overflow

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 28120 in the body.
You can then email your comments to 28120 AT debbugs.gnu.org in the normal way.

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

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

From: Kamil Dudka <kdudka <at> redhat.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] ptx: fix a possible crash caused by integer overflow
Date: Thu, 17 Aug 2017 13:40:24 +0200

The crash might not be reproducible in all environments but I was able
to observe invalid reads by valgrind while running ptx on the reproducer
for <https://bugzilla.redhat.com/1482445>:

% curl -s 'https://bugzilla.redhat.com/attachment.cgi?id=1314625' | gzip -cd | valgrind ptx > /dev/null
==4288== Memcheck, a memory error detector
==4288== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==4288== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==4288== Command: ptx
==4288==
==4288== Invalid read of size 1
==4288==    at 0x402FA9: define_all_fields (ptx.c:1434)
==4288==    by 0x402FA9: generate_all_output (ptx.c:1780)
==4288==    by 0x402FA9: main (ptx.c:2155)
==4288==  Address 0x5328a6f is 2,287 bytes inside a block of size 16,352 free'd
==4288==    at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==    by 0x408B65: xrealloc (xmalloc.c:61)
==4288==    by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==    by 0x402857: main (ptx.c:2141)
==4288==  Block was alloc'd at
==4288==    at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==    by 0x408B65: xrealloc (xmalloc.c:61)
==4288==    by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==    by 0x402857: main (ptx.c:2141)
==4288==
==4288== Invalid read of size 1
==4288==    at 0x4031AB: define_all_fields (ptx.c:1501)
==4288==    by 0x4031AB: generate_all_output (ptx.c:1780)
==4288==    by 0x4031AB: main (ptx.c:2155)
==4288==  Address 0x5328a6f is 2,287 bytes inside a block of size 16,352 free'd
==4288==    at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==    by 0x408B65: xrealloc (xmalloc.c:61)
==4288==    by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==    by 0x402857: main (ptx.c:2141)
==4288==  Block was alloc'd at
==4288==    at 0x4C2FC47: realloc (vg_replace_malloc.c:785)
==4288==    by 0x408B65: xrealloc (xmalloc.c:61)
==4288==    by 0x402857: find_occurs_in_text (ptx.c:955)
==4288==    by 0x402857: main (ptx.c:2141)
==4288==
==4288==
==4288== HEAP SUMMARY:
==4288==     in use at exit: 2,340,851 bytes in 96 blocks
==4288==   total heap usage: 200 allocs, 104 frees, 6,478,142 bytes allocated
==4288==
==4288== LEAK SUMMARY:
==4288==    definitely lost: 0 bytes in 0 blocks
==4288==    indirectly lost: 0 bytes in 0 blocks
==4288==      possibly lost: 0 bytes in 0 blocks
==4288==    still reachable: 2,340,851 bytes in 96 blocks
==4288==         suppressed: 0 bytes in 0 blocks
==4288== Rerun with --leak-check=full to see details of leaked memory
==4288==
==4288== For counts of detected and suppressed errors, rerun with: -v
==4288== ERROR SUMMARY: 915 errors from 2 contexts (suppressed: 0 from 0)
---
 src/ptx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ptx.c b/src/ptx.c
index c0c9733..f4ed7d4 100644
--- a/src/ptx.c
+++ b/src/ptx.c
@@ -224,7 +224,7 @@ static BLOCK *text_buffers;	/* files to study */
    start of the reference field, it is of type (DELTA) and usually
    negative.  */
 
-typedef short int DELTA;	/* to hold displacement within one context */
+typedef int DELTA;		/* to hold displacement within one context */
 
 typedef struct
   {
-- 
2.9.5

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Kamil Dudka <kdudka <at> redhat.com>, 28120-done <at> debbugs.gnu.org
Subject: Re: bug#28120: [PATCH] ptx: fix a possible crash caused by integer
 overflow
Date: Thu, 17 Aug 2017 12:14:05 -0700

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

On 08/17/2017 04:40 AM, Kamil Dudka wrote:
> -typedef short int DELTA;	/* to hold displacement within one context */
> +typedef int DELTA;		/* to hold displacement within one context */

Thanks for the heads-up. Although that fixes things for that particular 
test case, it won't work for larger cases. The type should be ptrdiff_t 
instead of int.

As its FIXME comment says, ptx is riddled with integer-overflow bugs. I 
installed the attached patch to fix the bug that you mentioned along 
with the other low-hanging fruit that I found, and am marking the bug as 
fixed upstream. I expect some other integer-overflow bugs can still 
occur in practice, but at least this patch is a significant improvement.

This patch prefers signed integer types like ptrdiff_t to unsigned types 
like size_t, as signed types allow for better checking when compiled 
with sanitization.

[0001-ptx-fix-some-integer-overflow-bugs.patch (text/x-patch, attachment)]

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

From: Kamil Dudka <kdudka <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-coreutils <at> gnu.org, 28120-done <at> debbugs.gnu.org
Subject: Re: bug#28120: [PATCH] ptx: fix a possible crash caused by integer
 overflow
Date: Fri, 18 Aug 2017 10:17:08 +0200

On Thursday, August 17, 2017 12:14:05 Paul Eggert wrote:
> On 08/17/2017 04:40 AM, Kamil Dudka wrote:
> > -typedef short int DELTA;	/* to hold displacement within one context */
> > +typedef int DELTA;		/* to hold displacement within one context */
> 
> Thanks for the heads-up. Although that fixes things for that particular
> test case, it won't work for larger cases.

Do you have an example of the larger case?  We could add a test-case for it.

> The type should be ptrdiff_t instead of int.
> 
> As its FIXME comment says, ptx is riddled with integer-overflow bugs. I
> installed the attached patch to fix the bug that you mentioned along
> with the other low-hanging fruit that I found, and am marking the bug as
> fixed upstream. I expect some other integer-overflow bugs can still
> occur in practice, but at least this patch is a significant improvement.
> 
> This patch prefers signed integer types like ptrdiff_t to unsigned types
> like size_t, as signed types allow for better checking when compiled
> with sanitization.

Your patch introduces the following warnings:

Error: CONSTANT_EXPRESSION_RESULT:
src/ptx.c:1939: result_independent_of_operands: "tmp <= 9223372036854775807L" is always true regardless of the values of its operands. This occurs as the logical second operand of "&&".
# 1937|               intmax_t tmp;
# 1938|               if (! (xstrtoimax (optarg, NULL, 0, &tmp, NULL) == LONGINT_OK
# 1939|->                    && 0 < tmp && tmp <= PTRDIFF_MAX))
# 1940|                 die (EXIT_FAILURE, 0, _("invalid gap width: %s"),
# 1941|                      quote (optarg));

Error: CONSTANT_EXPRESSION_RESULT:
src/ptx.c:1966: result_independent_of_operands: "tmp <= 9223372036854775807L" is always true regardless of the values of its operands. This occurs as the logical second operand of "&&".
# 1964|               intmax_t tmp;
# 1965|               if (! (xstrtoimax (optarg, NULL, 0, &tmp, NULL) == LONGINT_OK
# 1966|->                    && 0 < tmp && tmp <= PTRDIFF_MAX))
# 1967|                 die (EXIT_FAILURE, 0, _("invalid line width: %s"),
# 1968|                      quote (optarg));

Anyway, it fixes the original bug so I am fine with the patch as it is.

Thank you for pushing the fix!

Kamil

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Kamil Dudka <kdudka <at> redhat.com>
Cc: bug-coreutils <at> gnu.org, 28120-done <at> debbugs.gnu.org
Subject: Re: bug#28120: [PATCH] ptx: fix a possible crash caused by integer
 overflow
Date: Fri, 18 Aug 2017 08:44:11 -0700

Kamil Dudka wrote:

> Do you have an example of the larger case?  We could add a test-case for it.

I don't have an example. It would have to be pretty large, e.g., input 
containing more than 2**31 lines.

> Your patch introduces the following warnings:

These are Coverity warnings, right? They are false alarms, as usual for Coverity 
with this sort of thing.

Previous Next

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