* [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors @ 2011-01-25 1:33 Phillip Lougher 2011-01-25 17:09 ` Geert Uytterhoeven 0 siblings, 1 reply; 4+ messages in thread From: Phillip Lougher @ 2011-01-25 1:33 UTC (permalink / raw) To: Linux Kernel Development; +Cc: Jesper Juhl, Andrew Morton Fix potential use of uninitialised variable caused by recent decompressor code optimisations. In zlib_uncompress (zlib_wrapper.c) we have int zlib_err, zlib_init = 0; ... do { ... if (avail == 0) { offset = 0; put_bh(bh[k++]); continue; } ... zlib_err = zlib_inflate(stream, Z_SYNC_FLUSH); ... } while (zlib_err == Z_OK); If continue is executed (avail == 0) then the while condition will be evaluated testing zlib_err, which is uninitialised first time around the loop. Fix this by getting rid of the 'if (avail == 0)' condition test, this edge condition should not be being handled in the decompressor code, and instead handle it generically in the caller code. Similarly for xz_wrapper.c. Incidentally, on most architectures (bar Mips and Parisc), no uninitialised variable warning is generated by gcc, this is because the while condition test on continue is optimised out and not performed (when executing continue zlib_err has not been changed since entering the loop, and logically if the while condition was true previously, then it's still true). Reported-by: Jesper Juhl <jj@chaosbits.net> Signed-off-by: Phillip Lougher <phillip@lougher.demon.co.uk> --- fs/squashfs/block.c | 8 ++++++++ fs/squashfs/xz_wrapper.c | 6 ------ fs/squashfs/zlib_wrapper.c | 6 ------ 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c index 2fb2882..8ab48bc 100644 --- a/fs/squashfs/block.c +++ b/fs/squashfs/block.c @@ -63,6 +63,14 @@ static struct buffer_head *get_block_length(struct super_block *sb, *length = (unsigned char) bh->b_data[*offset] | (unsigned char) bh->b_data[*offset + 1] << 8; *offset += 2; + + if (*offset == msblk->devblksize) { + put_bh(bh); + bh = sb_bread(sb, ++(*cur_index)); + if (bh == NULL) + return NULL; + *offset = 0; + } } return bh; diff --git a/fs/squashfs/xz_wrapper.c b/fs/squashfs/xz_wrapper.c index 856756c..c4eb400 100644 --- a/fs/squashfs/xz_wrapper.c +++ b/fs/squashfs/xz_wrapper.c @@ -95,12 +95,6 @@ static int squashfs_xz_uncompress(struct squashfs_sb_info *msblk, void **buffer, if (!buffer_uptodate(bh[k])) goto release_mutex; - if (avail == 0) { - offset = 0; - put_bh(bh[k++]); - continue; - } - stream->buf.in = bh[k]->b_data + offset; stream->buf.in_size = avail; stream->buf.in_pos = 0; diff --git a/fs/squashfs/zlib_wrapper.c b/fs/squashfs/zlib_wrapper.c index 818a5e0..4661ae2 100644 --- a/fs/squashfs/zlib_wrapper.c +++ b/fs/squashfs/zlib_wrapper.c @@ -82,12 +82,6 @@ static int zlib_uncompress(struct squashfs_sb_info *msblk, void **buffer, if (!buffer_uptodate(bh[k])) goto release_mutex; - if (avail == 0) { - offset = 0; - put_bh(bh[k++]); - continue; - } - stream->next_in = bh[k]->b_data + offset; stream->avail_in = avail; offset = 0; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors 2011-01-25 1:33 [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors Phillip Lougher @ 2011-01-25 17:09 ` Geert Uytterhoeven 2011-01-25 18:57 ` Phillip Lougher 0 siblings, 1 reply; 4+ messages in thread From: Geert Uytterhoeven @ 2011-01-25 17:09 UTC (permalink / raw) To: Phillip Lougher; +Cc: Linux Kernel Development, Jesper Juhl, Andrew Morton On Tue, Jan 25, 2011 at 02:33, Phillip Lougher <phillip@lougher.demon.co.uk> wrote: > Fix potential use of uninitialised variable caused by recent decompressor > code optimisations. > > In zlib_uncompress (zlib_wrapper.c) we have > > int zlib_err, zlib_init = 0; > ... > do { > ... > if (avail == 0) { > offset = 0; > put_bh(bh[k++]); > continue; > } > ... > zlib_err = zlib_inflate(stream, Z_SYNC_FLUSH); > ... > } while (zlib_err == Z_OK); > > If continue is executed (avail == 0) then the while condition will be > evaluated testing zlib_err, which is uninitialised first time around the > loop. > > Fix this by getting rid of the 'if (avail == 0)' condition test, this > edge condition should not be being handled in the decompressor code, and > instead handle it generically in the caller code. > > Similarly for xz_wrapper.c. > > Incidentally, on most architectures (bar Mips and Parisc), no > uninitialised variable warning is generated by gcc, this is because > the while condition test on continue is optimised out and not performed > (when executing continue zlib_err has not been changed since entering the > loop, and logically if the while condition was true previously, then it's > still true). As this is a "do { ... } while (...);" construct and not a "while (...) { ... }" construct, the condition is not checked before doing the first iteration. Furthermore the "continue" may happen during the first iteration (this depends on parameters passed to the function), so the compiler cannot make any assumptions about the value of zlib_err, except that may be uninitialized. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors 2011-01-25 17:09 ` Geert Uytterhoeven @ 2011-01-25 18:57 ` Phillip Lougher 2011-01-25 20:45 ` Geert Uytterhoeven 0 siblings, 1 reply; 4+ messages in thread From: Phillip Lougher @ 2011-01-25 18:57 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: Linux Kernel Development, Jesper Juhl, Andrew Morton Geert Uytterhoeven wrote: > On Tue, Jan 25, 2011 at 02:33, Phillip Lougher > <phillip@lougher.demon.co.uk> wrote: >> Incidentally, on most architectures (bar Mips and Parisc), no >> uninitialised variable warning is generated by gcc, this is because >> the while condition test on continue is optimised out and not performed >> (when executing continue zlib_err has not been changed since entering the >> loop, and logically if the while condition was true previously, then it's >> still true). > > As this is a "do { ... } while (...);" construct and not a "while > (...) { ... }" construct, > the condition is not checked before doing the first iteration. Furthermore the > "continue" may happen during the first iteration (this depends on parameters > passed to the function), so the compiler cannot make any assumptions about the > value of zlib_err, except that may be uninitialized. > No, I've checked the assembly language produced - the while condition test has been optimised out in the case of continue. This is the assembly language output for x86_64, the relevant bits manually annotated by me, zlib_uncompress: .LFB1375: ... call mutex_lock_nested movl $0, 32(%r12) movl $0, 8(%r12) xorl %edx, %edx xorl %eax, %eax # jump to top of loop jmp .L12 .p2align 4,,10 .p2align 3 .L25: # whole if (stream->avail_in == 0 && k < b) { block of code moved out of # main loop, so continue can fall through to main loop # if (stream->avail_in == 0) testl %edx, %edx jne .L2 # wait_on_buffer(bh[k]); movq 32(%rsp), %rsi movq 48(%rsp), %rdx movslq %r13d,%rcx movq %rcx, 56(%rsp) movl $.LC0, %edi movl %eax, (%rsp) leaq (%rsi,%rcx,8), %rcx movl 8(%rdx), %edx movl $306, %esi movq (%rcx), %r8 movq %rcx, 8(%rsp) movl %edx, 68(%rsp) xorl %edx, %edx movq %r8, 16(%rsp) call __might_sleep movq 16(%rsp), %r8 movl (%rsp), %eax movq 8(%rsp), %rcx movq (%r8), %rdx andl $4, %edx jne .L24 .L3: # if (!buffer_uptodate(bh[k])) # goto release_mutex; movq (%rcx), %rdx movq (%rdx), %rcx andl $1, %ecx je .L4 # int avail = min(length, msblk->devblksize - offset); movl 68(%rsp), %ecx subl 44(%rsp), %ecx cmpl 28(%rsp), %ecx cmovg 28(%rsp), %ecx # length -= avail; subl %ecx, 28(%rsp) # if (avail == 0) testl %ecx, %ecx jne .L5 # put_bh(bh[k++]); addl $1, %r13d #APP # 107 "/stripe/git-trees/linux-linus-bugfix-1/arch/x86/include/asm/atomic.h" 1 .section .smp_locks,"a" .balign 4 .long 671f - . .previous 671: lock; decl 96(%rdx) # 0 "" 2 #NO_APP # offset = 0; movl $0, 44(%rsp) .p2align 4,,10 .p2align 3 # *continue* fails through to top of loop # no while condition test # top of loop .L6: movl 8(%r12), %edx # optimised case - first iteration around loop enters here as # edx doesn't need to be loaded (it already holds 0 from # stream->avail_in = 0) .L12: # if (k < b) cmpl %ebp, %r13d setl %r15b jl .L25 .L2: # if (stream->avail_out == 0 && page < pages) { cmpl 152(%rsp), %r14d jge .L7 movl 32(%r12), %ecx testl %ecx, %ecx jne .L7 movslq %r14d,%rdx movl $4096, 32(%r12) addl $1, %r14d movq (%rbx,%rdx,8), %rdx movq %rdx, 24(%r12) .L7: # if (!zlib_init) { testl %eax, %eax jne .L8 movl $15, %esi movq %r12, %rdi call zlib_inflateInit2 testl %eax, %eax jne .L26 .L8: # zlib_err = zlib_inflate(stream, Z_SYNC_FLUSH); movl $3, %esi movq %r12, %rdi call zlib_inflate # if( stream->avail_in == 0 && k < b) { testb %r15b, %r15b je .L10 movl 8(%r12), %edx testl %edx, %edx jne .L10 movq 32(%rsp), %rcx movslq %r13d,%rdx addl $1, %r13d movq (%rcx,%rdx,8), %rdx #APP # 107 "/stripe/git-trees/linux-linus-bugfix-1/arch/x86/include/asm/atomic.h" 1 .section .smp_locks,"a" .balign 4 .long 671f - . .previous 671: lock; decl 96(%rdx) # 0 "" 2 #NO_APP .L10: # } while (zlib_err == Z_OK); testl %eax, %eax jne .L11 movb $1, %al # jump to top of loop jmp .L6 .p2align 4,,10 .p2align 3 Phillip ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors 2011-01-25 18:57 ` Phillip Lougher @ 2011-01-25 20:45 ` Geert Uytterhoeven 0 siblings, 0 replies; 4+ messages in thread From: Geert Uytterhoeven @ 2011-01-25 20:45 UTC (permalink / raw) To: Phillip Lougher; +Cc: Linux Kernel Development, Jesper Juhl, Andrew Morton On Tue, Jan 25, 2011 at 19:57, Phillip Lougher <phillip@lougher.demon.co.uk> wrote: > Geert Uytterhoeven wrote: >> >> On Tue, Jan 25, 2011 at 02:33, Phillip Lougher >> <phillip@lougher.demon.co.uk> wrote: >>> >>> Incidentally, on most architectures (bar Mips and Parisc), no >>> uninitialised variable warning is generated by gcc, this is because >>> the while condition test on continue is optimised out and not performed >>> (when executing continue zlib_err has not been changed since entering the >>> loop, and logically if the while condition was true previously, then it's >>> still true). >> >> As this is a "do { ... } while (...);" construct and not a "while >> (...) { ... }" construct, >> the condition is not checked before doing the first iteration. Furthermore >> the >> "continue" may happen during the first iteration (this depends on >> parameters >> passed to the function), so the compiler cannot make any assumptions about >> the >> value of zlib_err, except that may be uninitialized. >> > > No, I've checked the assembly language produced - the while condition test > has > been optimised out in the case of continue. > > This is the assembly language output for x86_64, the relevant > bits manually annotated by me, While the continue skips the test at the end of the loop, that doesn't change the fact that zlib_err was never initialized in the first place. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-01-25 20:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-25 1:33 [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors Phillip Lougher 2011-01-25 17:09 ` Geert Uytterhoeven 2011-01-25 18:57 ` Phillip Lougher 2011-01-25 20:45 ` Geert Uytterhoeven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.