Fix UB in inffast.c when not using window

[atdt]

When not using window, window + wsize applies a zero offset to a null pointer, which is undefined behavior.

[mtl1979]

I don't see anything undefined in using null pointer in pointer arithmetics... As long as the variable size is well-defined (non-void pointer), the generated assembly will be correct, basically it either assembles to load + addition, or load effective address (LEA).

The real issue comes if the pointer is later dereferenced.

[atdt]

IANALL (I am not a language lawyer, but..) this was discussed in https://reviews.llvm.org/D67122 and the consensus was that any arithmetic on null pointers is undefined in C, even if you add zero. UBSan flags it.

[mtl1979]

@atdt Pointer arithmetics in C are a lot easier than with C++... As zlib and zlib-ng are both C, the code has no UB. Compiler doesn't know at compile-time that the pointer is NULL constant or has 0 offset, so it can't optimize away offset.

[atdt]

@mtl1979 According to the discussion I linked to above, this is only UB in C, and not in C++, where it would be well-defined.

I think the change improves readability, and it helps us adopt zlib-ng because otherwise we have to carry a patch that adds a __attribute__((no_sanitize("undefined"))) annotation to this code.

[mtl1979]

@atdt Well... I tested with two of the most common compilers and both return same predictable answer... I didn't try with Visual C++ as it is C++ compiler and not C compiler.

[turol]

@mtl1979 That means nothing. UB is dangerous because some future compiler will likely start using it for optimization and your "working" code suddenly crashes or misbehaves.

[mtl1979]

@turol Future compilers can't optimize the code differently unless they know for sure that both variables are zero. There is no chance for that to happen as it would already trigger a compiler warning when all warnings are enabled.

[atdt]

@mtl1979 I'm confused. Are you opposed to this change?

Upstream LLVM very definitely thinks this is UB. There is an explicit check for this in UBSan which flags this code in zlib-ng. Why quibble?

[mtl1979]

@atdt I'm saying it's false positive as there is no constant NULL or nullptr.

[atdt]

It's undefined behavior because the C language specification does not define how the compiler ought to behave when you apply a zero offset to a null pointer. What current compilers can or cannot do with this code is not a very good argument.

[mtl1979]

@atdt Offsets have nothing to do with C, so it's expected that C standard doesn't define it... C standard only covers arrays which have defined element size. C standard also says that undefined element size is assumed to equal to 1 byte, which would also apply to explicit NULL pointers, but not other pointers that point to address 0.

[codecov]

Codecov Report

Merging #1037 (b09a8fd) into develop (e128537) will decrease coverage by 2.06%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1037      +/-   ##
===========================================
- Coverage    78.32%   76.26%   -2.07%     
===========================================
  Files           85       85              
  Lines         8730     8725       -5     
  Branches      1392     1392              
===========================================
- Hits          6838     6654     -184     
- Misses        1357     1528     +171     
- Partials       535      543       +8     

Flag	Coverage Δ
macos_clang	68.06% <100.00%> (ø)
macos_gcc	66.70% <100.00%> (ø)
ubuntu_clang	67.14% <100.00%> (-6.23%)	⬇️
ubuntu_clang_debug	66.41% <100.00%> (-7.28%)	⬇️
ubuntu_clang_inflate_allow_invalid_dist	66.91% <100.00%> (-6.21%)	⬇️
ubuntu_clang_inflate_strict	67.14% <100.00%> (-6.22%)	⬇️
ubuntu_clang_mmap	67.14% <100.00%> (-6.20%)	⬇️
ubuntu_clang_msan	67.14% <100.00%> (-6.23%)	⬇️
ubuntu_clang_pigz	31.64% <100.00%> (-2.44%)	⬇️
ubuntu_clang_pigz_no_optim	37.73% <100.00%> (-0.45%)	⬇️
ubuntu_clang_pigz_no_threads	31.30% <100.00%> (-2.33%)	⬇️
ubuntu_clang_reduced_mem	67.40% <100.00%> (-6.24%)	⬇️
ubuntu_gcc	66.16% <100.00%> (-6.67%)	⬇️
ubuntu_gcc_aarch64	69.11% <100.00%> (-5.63%)	⬇️
ubuntu_gcc_aarch64_compat_no_opt	67.37% <100.00%> (-5.02%)	⬇️
ubuntu_gcc_aarch64_no_acle	68.07% <100.00%> (-5.95%)	⬇️
ubuntu_gcc_aarch64_no_neon	68.35% <100.00%> (-5.95%)	⬇️
ubuntu_gcc_armhf	69.08% <100.00%> (-5.65%)	⬇️
ubuntu_gcc_armhf_compat_no_opt	67.31% <100.00%> (-5.04%)	⬇️
ubuntu_gcc_armhf_no_acle	69.26% <100.00%> (-5.73%)	⬇️
ubuntu_gcc_armhf_no_neon	69.48% <100.00%> (-5.75%)	⬇️
ubuntu_gcc_armsf	69.10% <100.00%> (-5.65%)	⬇️
ubuntu_gcc_armsf_compat_no_opt	67.31% <100.00%> (-5.04%)	⬇️
ubuntu_gcc_compat_no_opt	68.79% <100.00%> (-4.71%)	⬇️
ubuntu_gcc_mingw_i686	0.00% <0.00%> (ø)
ubuntu_gcc_mingw_x86_64	0.00% <0.00%> (ø)
ubuntu_gcc_no_avx2	69.12% <100.00%> (-5.60%)	⬇️
ubuntu_gcc_no_pclmulqdq	64.31% <100.00%> (-7.10%)	⬇️
ubuntu_gcc_no_sse2	66.92% <100.00%> (-6.90%)	⬇️
ubuntu_gcc_no_sse4	66.18% <100.00%> (-5.54%)	⬇️
ubuntu_gcc_o3	66.81% <100.00%> (-5.23%)	⬇️
ubuntu_gcc_osb	68.40% <100.00%> (-7.46%)	⬇️
ubuntu_gcc_pigz	31.84% <100.00%> (-2.59%)	⬇️
ubuntu_gcc_pigz_aarch64	35.92% <100.00%> (-0.20%)	⬇️
ubuntu_gcc_ppc	67.02% <100.00%> (-5.69%)	⬇️
ubuntu_gcc_ppc64	70.21% <100.00%> (-5.41%)	⬇️
ubuntu_gcc_ppc64le	68.62% <100.00%> (-5.53%)	⬇️
ubuntu_gcc_ppc_no_power8	69.48% <100.00%> (-5.77%)	⬇️
ubuntu_gcc_s390x	68.65% <100.00%> (-5.40%)	⬇️
ubuntu_gcc_sparc64	70.57% <100.00%> (-5.82%)	⬇️
win64_gcc	∅ <ø> (∅)
win64_gcc_compat_no_opt	∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
inffast.c	88.69% <100.00%> (ø)
zutil_p.h	0.00% <0.00%> (-71.43%)	⬇️
zutil.c	21.42% <0.00%> (-60.72%)	⬇️
deflate.h	12.00% <0.00%> (-35.83%)	⬇️
match_tpl.h	75.38% <0.00%> (-23.85%)	⬇️
trees.c	73.77% <0.00%> (-22.68%)	⬇️
trees_emit.h	77.77% <0.00%> (-22.23%)	⬇️
inftrees.c	86.33% <0.00%> (-11.52%)	⬇️
insert_string_tpl.h	96.87% <0.00%> (-3.13%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e128537...b09a8fd. Read the comment docs.