distcode check in inflateCopy

[tbeu]

I see that lencode range is checked in inflateCopy:

https://github.com/madler/zlib/blob/ceadaf28dfa48dbf238a0ddb884d4c543b4170e8/inflate.c#L1468-L1470

Why is there no such check for distcode?

Background: A runtime error is reported.

inflate.c:1471:38: runtime error: index -12178934 out of bounds for type 'code[1444]'

\0 0x56b0ee3aafd8 in inflateCopy zlib/inflate.c:1471:38

    #\1 0x56b0eddb44eb in Mat_VarDuplicate [matio/src/mat.c:1552](https://github.com/tbeu/matio/blob/11cc1e46521fdc8d5e00db85ce494c957fff0be3/src/mat.c#L1552):27
    #\2 0x56b0eddaf3e6 in MatioRead(char const*) [matio/ossfuzz/matio_wrap.h:49](https://github.com/tbeu/matio/blob/11cc1e46521fdc8d5e00db85ce494c957fff0be3/ossfuzz/matio_wrap.h#L49):38
    #\3 0x56b0eddaf571 in LLVMFuzzerTestOneInput [matio/ossfuzz/matio_fuzzer.cpp:30](https://github.com/tbeu/matio/blob/11cc1e46521fdc8d5e00db85ce494c957fff0be3/ossfuzz/matio_fuzzer.cpp#L30):12

[madler]

Please provide code and data that results in this error. Thanks.

[tbeu]

Thanks for your respone. I updated the above call stack. The error is reported by ossfuzz (with a crafted file) where zlib is driven via matio.

I noticed that https://github.com/tbeu/matio/blob/11cc1e46521fdc8d5e00db85ce494c957fff0be3/src/inflate.c#L317-L323 is triggered, indicating a fread beyond EOF. I also checked the return value of inflate for Z_STREAM_END which was not triggered at all.

It's kind of hard to create a stand-alone test-case and scenario reproducing the erroneous behaviour. It's likely that the matio (as the caller) is using inflate + inflateCopy in a wrong way. Thanks again.

[madler]

Why is there no such check for distcode?

@tbeu To answer your question, either both lencode and distcode are pointing into codes[] in the state, or they are both pointing to static tables for fixed codes. Only one of them needs to be checked to determine if both pointers need to be converted to point to the codes[] in the new state.

Immediately upon writing the above paragraph, I thought of a place in the code where there can be a return from inflate() between setting lencode and distcode, leaving open the possibility of the error you report. This does not matter for the correctness of inflate, since distcode is not used until after it is set later. However your compiler injects code that complains for some reason about simple pointer arithmetic when it suggests that there might be an out-of-bound access sometime in the future, without there being any such access. (What compiler is this?)

Try replacing line 926 (or thereabouts) in inflate.c:

state->lencode = (const code FAR *)(state->next);

with:

state->lencode = state->distcode = (const code FAR *)(state->next);

and let me know if that remedies the issue. Thanks.

[tbeu]

let me know if that remedies the issue

Indeed, https://github.com/madler/zlib/compare/545f1949635949159fa6282e81712aec32b5d4f1...68370a6aeeeaa1f52d809f2d452feec968c0d550 resolves the issue. Thanks for your help!

Please close this issue once fixed in develop.

[madler]

Very good, thank you for testing it.

So what compiler / compiler options is checking for "index out of bounds" without indexing?

[tbeu]

So what compiler / compiler options is checking for "index out of bounds" without indexing?

It's UBSan of LLVM, but I do not know the exact version which is nowhere logged. Maybe @DavidKorczynski can answer it,

[madler]

Hmm. Looking at the documentation: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html , it seems to be going beyond what it claims. The documentation says "Out of bounds array indexing, in cases where the array bound can be statically determined." I don't see anything there about checking array-derived pointers against the array bounds when the pointers are never dereferenced.

[madler]

https://github.com/madler/zlib/commit/f7d01aae6ec6115184de821b93fa47810abd88f9