Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an off-by-one error that causes memory corruption. #69

Merged
merged 1 commit into from
May 12, 2017

Conversation

pkern
Copy link
Contributor

@pkern pkern commented May 11, 2017

We read up to bsize bytes from gInFile, so allocate as much memory.

We read up to bsize bytes from gInFile, so allocate as much memory.
@wookietreiber wookietreiber requested a review from vasi May 11, 2017 12:27
@vasi
Copy link
Owner

vasi commented May 11, 2017

It's more "off by 1-3" than "off by one", but it looks correct :)

@wookietreiber
Copy link
Collaborator

wookietreiber commented May 11, 2017

@pkern Do you have a test we can add? Idealy, the test would fail before the PR but succeed with the PR.

@pkern
Copy link
Contributor Author

pkern commented May 11, 2017

Unfortunately I don't have one. Compiled with tcmalloc it was very unhappy and it instantly crashed under ASan.

@vasi
Copy link
Owner

vasi commented May 11, 2017

Yeah, this would be a pretty difficult thing to test for, except under valgrind/ASan.

@wookietreiber
Copy link
Collaborator

Well, I started a valgrind PR in #41 but I didn't continue with the merge because I wanted to merge only when there were no more issues. When I find time I try to rebase #41 onto this one and see if / how it improves things.

@wookietreiber
Copy link
Collaborator

Confirmed. I just cherry-picked your commit on top of #41 and did a before after valgrind check.

Before:

==7192== Memcheck, a memory error detector
==7192== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==7192== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==7192== Command: ../src/pixz -d decompression-memcheck.sh.pixz decompression-memcheck.sh.uncompressed
==7192== 
==7192== Thread 10:
==7192== Invalid write of size 8
==7192==    at 0x4C2F4B3: memcpy@GLIBC_2.2.5 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x58B424E: _IO_file_xsgetn (in /usr/lib/libc-2.25.so)
==7192==    by 0x58A8BF8: fread (in /usr/lib/libc-2.25.so)
==7192==    by 0x404A81: read_thread (read.c:544)
==7192==    by 0x4029F9: pipeline_thread_split (common.c:513)
==7192==    by 0x56292E6: start_thread (in /usr/lib/libpthread-2.25.so)
==7192==  Address 0x6ee1ca0 is 384 bytes inside a block of size 390 alloc'd
==7192==    at 0x4C2AE4F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x4C2D1CF: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x4041B0: block_capacity (read.c:295)
==7192==    by 0x404A69: read_thread (read.c:541)
==7192==    by 0x4029F9: pipeline_thread_split (common.c:513)
==7192==    by 0x56292E6: start_thread (in /usr/lib/libpthread-2.25.so)
==7192== 
==7192== Thread 3:
==7192== Invalid read of size 2
==7192==    at 0x4C31A08: memmove (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x53FF52C: ??? (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x540618C: ??? (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x53FF7F0: lzma_code (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x403EEC: decode_thread (read.c:589)
==7192==    by 0x402A19: pipeline_thread_process (common.c:519)
==7192==    by 0x56292E6: start_thread (in /usr/lib/libpthread-2.25.so)
==7192==  Address 0x6ee1ca6 is 0 bytes after a block of size 390 alloc'd
==7192==    at 0x4C2AE4F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x4C2D1CF: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x4041B0: block_capacity (read.c:295)
==7192==    by 0x404A69: read_thread (read.c:541)
==7192==    by 0x4029F9: pipeline_thread_split (common.c:513)
==7192==    by 0x56292E6: start_thread (in /usr/lib/libpthread-2.25.so)
==7192== 
==7192== 
==7192== HEAP SUMMARY:
==7192==     in use at exit: 1,824 bytes in 9 blocks
==7192==   total heap usage: 88 allocs, 79 frees, 16,863,344 bytes allocated
==7192== 
==7192== Thread 1:
==7192== 112 bytes in 1 blocks are definitely lost in loss record 4 of 9
==7192==    at 0x4C2AF1F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x541498D: ??? (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x5406C84: lzma_filter_flags_decode (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x540675E: lzma_block_header_decode (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x4032A5: decode_file_index_start (common.c:106)
==7192==    by 0x4032A5: find_file_index.constprop.2 (common.c:128)
==7192==    by 0x403410: read_file_index (common.c:158)
==7192==    by 0x404EC5: pixz_read (read.c:99)
==7192==    by 0x40273C: main (pixz.c:166)
==7192== 
==7192== 112 bytes in 1 blocks are definitely lost in loss record 5 of 9
==7192==    at 0x4C2AF1F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x541498D: ??? (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x5406C84: lzma_filter_flags_decode (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x540675E: lzma_block_header_decode (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x403E8B: decode_thread (read.c:575)
==7192==    by 0x402A19: pipeline_thread_process (common.c:519)
==7192==    by 0x56292E6: start_thread (in /usr/lib/libpthread-2.25.so)
==7192== 
==7192== 168 (96 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss record 7 of 9
==7192==    at 0x4C2AF1F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7192==    by 0x53FF65D: ??? (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x5407147: lzma_index_decoder (in /usr/lib/liblzma.so.5.2.3)
==7192==    by 0x402D5B: next_index (common.c:315)
==7192==    by 0x4030E9: decode_index (common.c:356)
==7192==    by 0x404B10: pixz_read (read.c:97)
==7192==    by 0x40273C: main (pixz.c:166)
==7192== 
==7192== LEAK SUMMARY:
==7192==    definitely lost: 320 bytes in 3 blocks
==7192==    indirectly lost: 72 bytes in 1 blocks
==7192==      possibly lost: 0 bytes in 0 blocks
==7192==    still reachable: 1,432 bytes in 5 blocks
==7192==         suppressed: 0 bytes in 0 blocks
==7192== Reachable blocks (those to which a pointer was found) are not shown.
==7192== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==7192== 
==7192== For counts of detected and suppressed errors, rerun with: -v
==7192== ERROR SUMMARY: 5 errors from 5 contexts (suppressed: 0 from 0)

After:

==8072== Memcheck, a memory error detector
==8072== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==8072== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==8072== Command: ../src/pixz -d decompression-memcheck.sh.pixz decompression-memcheck.sh.uncompressed
==8072== 
==8072== 
==8072== HEAP SUMMARY:
==8072==     in use at exit: 1,824 bytes in 9 blocks
==8072==   total heap usage: 88 allocs, 79 frees, 16,863,346 bytes allocated
==8072== 
==8072== 112 bytes in 1 blocks are definitely lost in loss record 4 of 9
==8072==    at 0x4C2AF1F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8072==    by 0x541498D: ??? (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x5406C84: lzma_filter_flags_decode (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x540675E: lzma_block_header_decode (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x4032A5: decode_file_index_start (common.c:106)
==8072==    by 0x4032A5: find_file_index.constprop.2 (common.c:128)
==8072==    by 0x403410: read_file_index (common.c:158)
==8072==    by 0x404EC5: pixz_read (read.c:99)
==8072==    by 0x40273C: main (pixz.c:166)
==8072== 
==8072== 112 bytes in 1 blocks are definitely lost in loss record 5 of 9
==8072==    at 0x4C2AF1F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8072==    by 0x541498D: ??? (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x5406C84: lzma_filter_flags_decode (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x540675E: lzma_block_header_decode (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x403E8B: decode_thread (read.c:575)
==8072==    by 0x402A19: pipeline_thread_process (common.c:519)
==8072==    by 0x56292E6: start_thread (in /usr/lib/libpthread-2.25.so)
==8072== 
==8072== 168 (96 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss record 7 of 9
==8072==    at 0x4C2AF1F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8072==    by 0x53FF65D: ??? (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x5407147: lzma_index_decoder (in /usr/lib/liblzma.so.5.2.3)
==8072==    by 0x402D5B: next_index (common.c:315)
==8072==    by 0x4030E9: decode_index (common.c:356)
==8072==    by 0x404B10: pixz_read (read.c:97)
==8072==    by 0x40273C: main (pixz.c:166)
==8072== 
==8072== LEAK SUMMARY:
==8072==    definitely lost: 320 bytes in 3 blocks
==8072==    indirectly lost: 72 bytes in 1 blocks
==8072==      possibly lost: 0 bytes in 0 blocks
==8072==    still reachable: 1,432 bytes in 5 blocks
==8072==         suppressed: 0 bytes in 0 blocks
==8072== Reachable blocks (those to which a pointer was found) are not shown.
==8072== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==8072== 
==8072== For counts of detected and suppressed errors, rerun with: -v
==8072== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

I am merging. @pkern Thank you for the contribution.

@wookietreiber wookietreiber merged commit 943932c into vasi:master May 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants