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

Buffer overflow when compressing some 16 bits images of the test suite #539

Closed
mayeut opened this issue Jul 19, 2015 · 5 comments
Closed
Labels

Comments

@mayeut
Copy link
Collaborator

mayeut commented Jul 19, 2015

Following merge of PR #538 (new tests have been added, it's not caused by code addition), we can see that compressing some 16bits images create a buffer overflow.

http://my.cdash.org/viewDynamicAnalysisFile.php?id=3153754 :

=================================================================
heap-buffer-overflow ==76313==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000df74 at pc 0x000108c5f44a bp 0x7fff57006370 sp 0x7fff57006368
WRITE of size 1 at 0x60200000df74 thread T0
    #0 0x108c5f449 in opj_mqc_byteout /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/mqc.c:214:4
    #1 0x108c5ef86 in opj_mqc_flush /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/mqc.c:417:2
    #2 0x108c7445a in opj_t1_encode_cblk /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/t1.c:1682:3
    #3 0x108c73446 in opj_t1_encode_cblks /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/t1.c:1536:7
    #4 0x108c87f49 in opj_tcd_t1_encode /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/tcd.c:2045:15
    #5 0x108c872fe in opj_tcd_encode_tile /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/tcd.c:1235:23
    #6 0x108c4cd2c in opj_j2k_write_sod /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/j2k.c:4397:15
    #7 0x108c4bed1 in opj_j2k_write_first_tile_part /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/j2k.c:10447:15
    #8 0x108c435b7 in opj_j2k_post_write_tile /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/j2k.c:10287:15
    #9 0x108c4247e in opj_j2k_encode /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/j2k.c:10047:23
    #10 0x108bf503a in main /Users/Matt/Dev/OpenJpegNew/openjpeg/src/bin/jp2/opj_compress.c:1836:36
    #11 0x7fff9732d5c8 in start (/usr/lib/system/libdyld.dylib+0x35c8)
    #12 0x4 (<unknown module>)

0x60200000df74 is located 0 bytes to the right of 4-byte region [0x60200000df70,0x60200000df74)
allocated by thread T0 here:
    #0 0x108f11ccf in wrap_malloc (/Users/Matt/Dev/llvm-clang-3.5.0-macosx-apple-darwin/lib/clang/3.5.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x2bccf)
    #1 0x108c8b0a0 in opj_tcd_code_block_enc_allocate_data /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/tcd.c:1092:36
    #2 0x108c861a8 in opj_tcd_init_tile /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/tcd.c:1017:14
    #3 0x108c4282c in opj_j2k_pre_write_tile /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/j2k.c:10139:15
    #4 0x108c421e9 in opj_j2k_encode /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/j2k.c:9995:23
    #5 0x108bf503a in main /Users/Matt/Dev/OpenJpegNew/openjpeg/src/bin/jp2/opj_compress.c:1836:36
    #6 0x7fff9732d5c8 in start (/usr/lib/system/libdyld.dylib+0x35c8)
    #7 0x4 (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow /Users/Matt/Dev/OpenJpegNew/openjpeg/src/lib/openjp2/mqc.c:214 opj_mqc_byteout
Shadow bytes around the buggy address:
  0x1c0400001b90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0400001ba0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0400001bb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0400001bc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0400001bd0: fa fa fa fa fa fa fa fa fa fa 00 00 fa fa 00 00
=>0x1c0400001be0: fa fa 00 00 fa fa 04 fa fa fa 04 fa fa fa[04]fa
  0x1c0400001bf0: fa fa 04 fa fa fa 00 fa fa fa fd fd fa fa 00 04
  0x1c0400001c00: fa fa 04 fa fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x1c0400001c10: fa fa 00 00 fa fa 00 06 fa fa 00 00 fa fa 00 04
  0x1c0400001c20: fa fa 00 06 fa fa 00 04 fa fa 00 fa fa fa 00 00
  0x1c0400001c30: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
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
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  ASan internal:           fe
==76313==ABORTING
@mayeut
Copy link
Collaborator Author

mayeut commented Jul 19, 2015

I think the allocation of code blocks is off by 1:

/**
 * Allocates data memory for an encoding code block.
 */
static OPJ_BOOL opj_tcd_code_block_enc_allocate_data (opj_tcd_cblk_enc_t * p_code_block)
{
    OPJ_UINT32 l_data_size;

    l_data_size = (OPJ_UINT32)((p_code_block->x1 - p_code_block->x0) * (p_code_block->y1 - p_code_block->y0) * (OPJ_INT32)sizeof(OPJ_UINT32));

    if (l_data_size > p_code_block->data_size) {
        if (p_code_block->data) {
            opj_free(p_code_block->data - 1); /* again, why -1 */
        }
        p_code_block->data = (OPJ_BYTE*) opj_malloc(l_data_size); /* HERE MISSING ONE */
        if(! p_code_block->data) {
            p_code_block->data_size = 0U;
            return OPJ_FALSE;
        }
        p_code_block->data_size = l_data_size;
        p_code_block->data[0] = 0;
        p_code_block->data+=1;   /*why +1 ?*/
    }
    return OPJ_TRUE;
}

shall probably be

/**
 * Allocates data memory for an encoding code block.
 */
static OPJ_BOOL opj_tcd_code_block_enc_allocate_data (opj_tcd_cblk_enc_t * p_code_block)
{
    OPJ_UINT32 l_data_size;

    l_data_size = (OPJ_UINT32)((p_code_block->x1 - p_code_block->x0) * (p_code_block->y1 - p_code_block->y0) * (OPJ_INT32)sizeof(OPJ_UINT32));

    if (l_data_size > p_code_block->data_size) {
        if (p_code_block->data) {
            opj_free(p_code_block->data - 1); /* again, why -1 */
        }
        p_code_block->data = (OPJ_BYTE*) opj_malloc(l_data_size+1); /* HERE */
        if(! p_code_block->data) {
            p_code_block->data_size = 0U;
            return OPJ_FALSE;
        }
        p_code_block->data_size = l_data_size;
        p_code_block->data[0] = 0;
        p_code_block->data+=1;   /*why +1 ?*/
    }
    return OPJ_TRUE;
}

Or maybe the initial +1 to data can be removed.
Any thoughts ?

@mayeut mayeut added the bug label Jul 19, 2015
@mayeut
Copy link
Collaborator Author

mayeut commented Jul 20, 2015

The initial + 1 probably can't be removed as mqc is using data - 1 on init.

@mayeut
Copy link
Collaborator Author

mayeut commented Jul 21, 2015

@detonin, I prevented the overflow for this specific case.
I won't close the issue until someone with a better understanding validates or asks to rework.

@mayeut
Copy link
Collaborator Author

mayeut commented Jul 25, 2015

Seems to be related to #259
Seems to be related to #5
Seems to be related to #62

@rouault
Copy link
Collaborator

rouault commented Jul 29, 2017

Closing. mayeut@9ac3a15 is reasonable

@rouault rouault closed this as completed Jul 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants