[CHUNKCOPY_SAFE] Fix off-by-one error #982

mtl1979 · 2021-06-01T22:02:11Z

When chunk size is more than 8 bytes, the comparison logic failed if remaining buffer length was one less than chunk size.

For example when using AVX2 with buffer length of 31 bytes, it would write past end of buffer.
When len is less than remaining buffer length, CHUNKCOPY_SAFE now uses smaller chunks to avoid writing past of end of buffer.

codecov · 2021-06-01T22:06:15Z

Codecov Report

Merging #982 (8710f63) into develop (c58d995) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 8710f63 differs from pull request most recent head bd27bef. Consider uploading reports for the commit bd27bef to get more accurate results

@@             Coverage Diff             @@
##           develop     #982      +/-   ##
===========================================
- Coverage    76.42%   76.32%   -0.10%     
===========================================
  Files           74       74              
  Lines         8305     8313       +8     
  Branches      1369     1370       +1     
===========================================
- Hits          6347     6345       -2     
- Misses        1427     1439      +12     
+ Partials       531      529       -2

Flag	Coverage Δ
macos_clang	`68.40% <100.00%> (-0.09%)`	⬇️
macos_gcc	`67.35% <100.00%> (-0.06%)`	⬇️
ubuntu_clang	`69.35% <100.00%> (-0.09%)`	⬇️
ubuntu_clang_debug	`68.74% <100.00%> (-0.11%)`	⬇️
ubuntu_clang_inflate_allow_invalid_dist	`69.09% <100.00%> (-0.09%)`	⬇️
ubuntu_clang_inflate_strict	`69.34% <100.00%> (-0.09%)`	⬇️
ubuntu_clang_mmap	`69.07% <100.00%> (-0.09%)`	⬇️
ubuntu_clang_msan	`69.35% <100.00%> (-0.09%)`	⬇️
ubuntu_clang_pigz	`35.01% <25.00%> (?)`
ubuntu_clang_pigz_no_optim	`37.67% <25.00%> (-0.08%)`	⬇️
ubuntu_clang_pigz_no_threads	`34.74% <25.00%> (?)`
ubuntu_gcc	`68.46% <100.00%> (-0.20%)`	⬇️
ubuntu_gcc_aarch64	`68.73% <100.00%> (-0.21%)`	⬇️
ubuntu_gcc_aarch64_compat_no_opt	`67.11% <100.00%> (-0.13%)`	⬇️
ubuntu_gcc_aarch64_no_acle	`67.63% <100.00%> (-0.21%)`	⬇️
ubuntu_gcc_aarch64_no_neon	`68.12% <100.00%> (-0.12%)`	⬇️
ubuntu_gcc_armhf	`68.71% <100.00%> (-0.21%)`	⬇️
ubuntu_gcc_armhf_compat_no_opt	`67.22% <100.00%> (-0.01%)`	⬇️
ubuntu_gcc_armhf_no_acle	`68.88% <100.00%> (-0.22%)`	⬇️
ubuntu_gcc_armhf_no_neon	`69.28% <100.00%> (-0.14%)`	⬇️
ubuntu_gcc_armsf	`68.73% <100.00%> (-0.21%)`	⬇️
ubuntu_gcc_armsf_compat_no_opt	`67.22% <100.00%> (-0.01%)`	⬇️
ubuntu_gcc_compat_no_opt	`68.48% <100.00%> (-0.14%)`	⬇️
ubuntu_gcc_mingw_i686	`0.00% <0.00%> (ø)`
ubuntu_gcc_mingw_x86_64	`0.00% <0.00%> (ø)`
ubuntu_gcc_no_avx2	`68.85% <100.00%> (-0.17%)`	⬇️
ubuntu_gcc_no_pclmulqdq	`66.63% <100.00%> (-0.21%)`	⬇️
ubuntu_gcc_no_sse2	`67.86% <100.00%> (-0.20%)`	⬇️
ubuntu_gcc_no_sse4	`66.59% <100.00%> (-0.21%)`	⬇️
ubuntu_gcc_o3	`68.20% <100.00%> (-0.19%)`	⬇️
ubuntu_gcc_osb	`70.74% <100.00%> (-0.21%)`	⬇️
ubuntu_gcc_pigz	`34.80% <25.00%> (?)`
ubuntu_gcc_ppc	`69.09% <100.00%> (-0.03%)`	⬇️
ubuntu_gcc_ppc64	`69.88% <100.00%> (-0.03%)`	⬇️
ubuntu_gcc_ppc64le	`68.47% <100.00%> (-0.13%)`	⬇️
ubuntu_gcc_s390x	`67.85% <100.00%> (-0.01%)`	⬇️
ubuntu_gcc_sparc64	`70.28% <100.00%> (-0.03%)`	⬇️
win64_gcc	`70.08% <12.50%> (-0.23%)`	⬇️
win64_gcc_compat_no_opt	`73.71% <100.00%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
chunkset_tpl.h	`99.11% <100.00%> (+0.08%)`	⬆️
inffast.c	`88.27% <100.00%> (ø)`
functable.c	`70.31% <0.00%> (-10.94%)`	⬇️
arch/arm/chunkset_neon.c	`100.00% <0.00%> (ø)`
trees.c	`74.86% <0.00%> (+1.08%)`	⬆️

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 c58d995...bd27bef. Read the comment docs.

mtl1979 · 2021-06-01T23:01:45Z

@nmoinvaz

nmoinvaz · 2021-06-02T00:45:15Z

It does not fix #979. I'm not sure I understand the off-by-one error. if (31 < sizeof(chunk_t)) (only 31 bytes would be written) - 16, 8, 4, 2, 1 = 31. However I do like the idea of moving MIN to zbuild.h so it can be used by other sources.

mtl1979 · 2021-06-02T00:57:41Z

@nmoinvaz I think there might be error in code calling CHUNKCOPY_SAFE somewhere as otherwise the MIN() would be unnecessary... If I remove the MIN()... It fails with buffer size of 31, trying to write 32 bytes.

nmoinvaz · 2021-06-02T01:34:58Z

I don't think there is any off by one issue. Having the MIN() is good. But to properly do the limit this is what code I was playing around with. It didn't make a difference with the #979 but it does prevent limiting of the output.

I haven't tested it very well. It always makes sure that CHUNKCOPY has a multiple of chunksize. Because it is possible in the original function for say (safe - out) = 43. The original function would then write 32 * 2 or 64 bytes or past the safe buffer. It does probably decrease performance a bit because it has to do multiple memcpy's to get the initial alignment.

What do you think?

nmoinvaz · 2021-06-02T01:47:31Z

It also has to be combined with this change in chunkmemset:

This may be the off by one issue you are noticing. I was able to find this one by adding these "asserts":

mtl1979 · 2021-06-02T01:51:53Z

I was thinking of adding

len = MIN(len, safe - out + 1);

to top of CHUNKCOPY_SAFE. That would make the comparison cleaner...

nmoinvaz · 2021-06-02T23:30:38Z

@mtl1979 are you able to make the MIN() move into its own PR? There are many places in inflate code that could also benefit from it.

mtl1979 · 2021-06-02T23:36:07Z

@nmoinvaz I don't think separating the move of MIN() is hard...

mtl1979 · 2021-06-04T19:02:09Z

Rebased.

nmoinvaz · 2021-06-04T19:21:08Z

I still don't understand why it is off by one.

mtl1979 · 2021-06-04T19:43:27Z

@nmoinvaz

If you have buffer of 31 bytes, it is indexed from 0 to 30... That is where the - 1 comes from in one of the calling functions. I tested it by removing the + 1inside MIN() and tests started failing. It is only relevant when both len and the safe length are less than size of chunk_t but not equal.

Safe length can calculated by formula 30 (end index) - 0 (start index) + 1 = 31.

In this case it comes to the comparison, which is < and not <=... It has to fall through if safe length is same as size of chunk_t.

Dead2 · 2021-06-08T18:53:44Z

Please rebase, so we get the new CI-tests too :)

nmoinvaz · 2021-06-08T19:06:46Z

Benchmarks should be done as well.

mtl1979 · 2021-06-10T19:48:41Z

On my benchmark, difference between unrolling and not unrolling in chunkmemset_safe() was 23 ms using 211957760 bytes long file.

nmoinvaz · 2021-06-11T03:53:00Z

@mtl1979 you may be able to cherry pick f5325ab for the speed gains.

nmoinvaz · 2021-06-11T03:56:06Z

I think this "off-by-one" bug should be its own PR with 1 commit and anything else should be a separate PR.

chunkset_tpl.h

mtl1979

@mtl1979 you may be able to cherry pick f5325ab for the speed gains.

I don't see any speed gain on my machine... The extra branch is in speed sensitive location, so it actually gets slower.

mtl1979 · 2021-06-11T06:56:32Z

I think this "off-by-one" bug should be its own PR with 1 commit and anything else should be a separate PR.

Maybe... I just wanted to keep them together to test for any side-effects... If I separate, the "remaining" part must be merged in before the "off-by-one" fix. Then I can just rebase over for a really clean diff.

* When chunk size was more than 8 bytes, the comparison logic failed if safe length was one less than chunk size.

…to avoid errors with optimizations enabled.

* limit len to minimum of len and left

Dead2 · 2021-06-11T11:15:07Z

LGTM, but I am not so steady on the intricacies of the chunkset code, so I have not verified its correctness but rather its scope.

mtl1979 · 2021-06-11T11:37:20Z

@Dead2 From the start nobody was really sure about correctness of the chunkset code, they just assumed it was correct because it didn't crash or fail any CI tests back then... After adding pigz test, this is the first pull request that doesn't fail it and does not introduce new failures either. Codecov test fails only because this pull request replaces direct calls to two functions with indirect calls through another set of two functions.

It's really hard to spot off-by-one errors in any code and even more when there is guard bytes added by user application instead of the C library.

Dead2 · 2021-06-11T11:43:43Z

@mtl1979 I am aware. I meant that I'll need to wait for @nmoinvaz or someone else to approve as well. 😄

mtl1979 · 2021-06-11T11:45:15Z

@Dead2 I know... Controversial patches require two long-time collaborators... I already asked @nmoinvaz to recheck ;)

Dead2 · 2021-06-11T17:50:37Z

Baseline `5cfb7f1`

   text    data     bss     dec     hex filename
 111264    1296      32  112592   1b7d0 libz-ng.so.2

 Level   Comp   Comptime min/avg/max/stddev  Decomptime min/avg/max/stddev  Compressed size
 1     57.780%      1.874/1.898/1.908/0.008        1.057/1.095/1.107/0.010       90,924,650
 2     43.876%      2.032/2.045/2.051/0.005        0.690/0.702/0.708/0.005       41,427,218
 3     42.490%      2.093/2.109/2.118/0.007        0.562/0.570/0.574/0.003       33,432,016
 4     41.467%      2.021/2.042/2.052/0.009        0.432/0.443/0.448/0.004       26,101,482
 5     41.212%      2.158/2.199/2.209/0.010        0.432/0.441/0.446/0.004       25,940,920
 6     41.039%      1.922/1.933/1.940/0.005        0.321/0.331/0.336/0.004       19,374,021
 7     40.781%      1.658/1.666/1.671/0.004        0.210/0.218/0.221/0.003       12,834,980
 8     40.706%      2.074/2.084/2.090/0.006        0.212/0.217/0.221/0.002       12,811,166
 9     40.695%      2.264/2.280/2.286/0.005        0.209/0.218/0.222/0.003       12,807,941

 avg1  43.339%                        2.028                          0.471
 tot                                547.633                        127.059      275,654,394

PR #982 eb212936dd6da4222f566ea82f8ad48665aa614c

   text    data     bss     dec     hex filename
 111456    1296      32  112784   1b890 libz-ng.so.2

 Level   Comp   Comptime min/avg/max/stddev  Decomptime min/avg/max/stddev  Compressed size
 1     57.780%      1.882/1.902/1.910/0.006        1.263/1.280/1.285/0.005       90,924,650
 2     43.876%      2.029/2.041/2.047/0.005        0.814/0.827/0.833/0.004       41,427,218
 3     42.490%      2.091/2.109/2.116/0.006        0.657/0.677/0.683/0.006       33,432,016
 4     41.467%      2.021/2.041/2.049/0.007        0.517/0.527/0.533/0.004       26,101,482
 5     41.212%      2.133/2.195/2.207/0.013        0.517/0.525/0.530/0.004       25,940,920
 6     41.039%      1.916/1.930/1.939/0.006        0.386/0.392/0.395/0.002       19,374,021
 7     40.781%      1.654/1.663/1.669/0.005        0.249/0.259/0.263/0.003       12,834,980
 8     40.706%      2.071/2.086/2.093/0.006        0.243/0.258/0.262/0.004       12,811,166
 9     40.695%      2.261/2.279/2.287/0.008        0.251/0.258/0.261/0.003       12,807,941

 avg1  43.339%                        2.027                          0.556
 tot                                547.381                        150.102      275,654,394

- Fix inflate corruption #982 - Minor code cleanup #983 #984 - Fix mpicc compilation #959 - Fix build on NetBSD #964 - Fix build on OpenBSD #970 - Fix build on Cygwin #972 #974 - Fix linter warnings in configure #975 - Spelling fixes #961 - Improve unistd.h handling #960 - Remove stdarg.h detection #976 - CI/Test improvements #977 #981 #985 - Cmake improvements #980 #989

nmoinvaz · 2021-06-12T04:07:10Z

I am getting some warnings now on MSVC:

C:\Users\Nathan\Source\zlib-ng\chunkset_tpl.h(40,35): warning C4244: '=': conversion from '__int64' to 'unsigned int',
possible loss of data [C:\Users\Nathan\Source\zlib-ng\build-x\zlib.vcxproj]
C:\Users\Nathan\Source\zlib-ng\inffast.c(267,88): warning C4244: 'function': conversion from '__int64' to 'unsigned int
', possible loss of data [C:\Users\Nathan\Source\zlib-ng\build-x\zlib.vcxproj]
C:\Users\Nathan\Source\zlib-ng\chunkset_tpl.h(40,35): warning C4244: '=': conversion from '__int64' to 'unsigned int',
possible loss of data [C:\Users\Nathan\Source\zlib-ng\build-x\zlib.vcxproj]
C:\Users\Nathan\Source\zlib-ng\chunkset_tpl.h(40,35): warning C4244: '=': conversion from '__int64' to 'unsigned int',
possible loss of data [C:\Users\Nathan\Source\zlib-ng\build-x\zlib.vcxproj]

mtl1979 · 2021-06-12T04:39:42Z

@nmoinvaz That's because I didn't change the function signatures to use z_size_t instead of unsigned int...

mtl1979 · 2021-06-12T05:08:26Z

Casting the pointer difference to unsigned int would have introduced possible wraparound bug, as the result is allowed to be -1. Any negative difference except -1 is expected to crash.

nmoinvaz · 2021-06-12T07:20:35Z

How is it allowed to be -1?

mtl1979 · 2021-06-12T12:23:33Z

@nmoinvaz Because 1 is added to the pointer difference, -1 as difference means the passed-in buffer is full. Because it is now using the safe version of both chunkcopy and chunkmemset, it is allowed (even if it doesn't ever happen in normal circumstances) and essentially no-op. -2 as difference would result in all bits set in safe length, which would most likely overflow.

mtl1979 force-pushed the off-by-one branch 2 times, most recently from b10175f to 0316dd6 Compare June 1, 2021 22:21

mtl1979 changed the title ~~[CHUNKSET_SAFE] Fix off-by-one error~~ [CHUNKCOPY_SAFE] Fix off-by-one error Jun 1, 2021

mtl1979 force-pushed the off-by-one branch from 0316dd6 to e2c6232 Compare June 1, 2021 23:28

mtl1979 mentioned this pull request Jun 2, 2021

Move MIN() macro to zbuild.h #984

Merged

mtl1979 marked this pull request as draft June 3, 2021 00:08

mtl1979 force-pushed the off-by-one branch 3 times, most recently from b11e14f to 75288eb Compare June 4, 2021 17:19

Dead2 added the Rebase needed Please do a 'git rebase develop yourbranch' label Jun 4, 2021

mtl1979 force-pushed the off-by-one branch from 75288eb to e6fc00d Compare June 4, 2021 19:00

Dead2 removed the Rebase needed Please do a 'git rebase develop yourbranch' label Jun 4, 2021

mtl1979 marked this pull request as ready for review June 4, 2021 19:03

nmoinvaz mentioned this pull request Jun 5, 2021

Improvements to inflate chunkset code #987

Closed

Dead2 added the Rebase needed Please do a 'git rebase develop yourbranch' label Jun 8, 2021

Dead2 added the Benchmarks wanted Please benchmark with and without this PR label Jun 8, 2021

nmoinvaz reviewed Jun 11, 2021

View reviewed changes

chunkset_tpl.h Outdated Show resolved Hide resolved

mtl1979 commented Jun 11, 2021

View reviewed changes

[CHUNKCOPY_SAFE] Fix off-by-one error

d9f4ddd

* When chunk size was more than 8 bytes, the comparison logic failed if safe length was one less than chunk size.

mtl1979 force-pushed the off-by-one branch from f64b28a to a89f3b9 Compare June 11, 2021 07:51

[inflate_fast] Always use safe versions of chunkcopy and chunkmemset …

0373f6f

…to avoid errors with optimizations enabled.

mtl1979 force-pushed the off-by-one branch from a89f3b9 to cb110df Compare June 11, 2021 07:53

[CHUNKMEMSET_SAFE] Precalculate "from".

bd27bef

* limit len to minimum of len and left

mtl1979 force-pushed the off-by-one branch from cb110df to bd27bef Compare June 11, 2021 08:04

mtl1979 requested a review from nmoinvaz June 11, 2021 09:46

nmoinvaz approved these changes Jun 11, 2021

View reviewed changes

Dead2 merged commit 4af20ea into zlib-ng:develop Jun 11, 2021

Dead2 mentioned this pull request Jun 11, 2021

Version 2.0.4 #992

Merged

Dead2 removed the Benchmarks wanted Please benchmark with and without this PR label Jun 11, 2021

nmoinvaz mentioned this pull request Jun 19, 2021

SSE2/3/4/AVX2 optimisations cause crc32 error in pigz #979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CHUNKCOPY_SAFE] Fix off-by-one error #982

[CHUNKCOPY_SAFE] Fix off-by-one error #982

mtl1979 commented Jun 1, 2021 •

edited

codecov bot commented Jun 1, 2021 •

edited

mtl1979 commented Jun 1, 2021

nmoinvaz commented Jun 2, 2021 •

edited

mtl1979 commented Jun 2, 2021 •

edited

nmoinvaz commented Jun 2, 2021 •

edited

nmoinvaz commented Jun 2, 2021 •

edited

mtl1979 commented Jun 2, 2021

nmoinvaz commented Jun 2, 2021

mtl1979 commented Jun 2, 2021

mtl1979 commented Jun 4, 2021

nmoinvaz commented Jun 4, 2021

mtl1979 commented Jun 4, 2021

Dead2 commented Jun 8, 2021

nmoinvaz commented Jun 8, 2021

mtl1979 commented Jun 10, 2021

nmoinvaz commented Jun 11, 2021

nmoinvaz commented Jun 11, 2021 •

edited

mtl1979 left a comment

mtl1979 commented Jun 11, 2021

Dead2 commented Jun 11, 2021

mtl1979 commented Jun 11, 2021

Dead2 commented Jun 11, 2021

mtl1979 commented Jun 11, 2021

Dead2 commented Jun 11, 2021

nmoinvaz commented Jun 12, 2021 •

edited

mtl1979 commented Jun 12, 2021

mtl1979 commented Jun 12, 2021

nmoinvaz commented Jun 12, 2021

mtl1979 commented Jun 12, 2021

[CHUNKCOPY_SAFE] Fix off-by-one error #982

[CHUNKCOPY_SAFE] Fix off-by-one error #982

Conversation

mtl1979 commented Jun 1, 2021 • edited

codecov bot commented Jun 1, 2021 • edited

Codecov Report

mtl1979 commented Jun 1, 2021

nmoinvaz commented Jun 2, 2021 • edited

mtl1979 commented Jun 2, 2021 • edited

nmoinvaz commented Jun 2, 2021 • edited

nmoinvaz commented Jun 2, 2021 • edited

mtl1979 commented Jun 2, 2021

nmoinvaz commented Jun 2, 2021

mtl1979 commented Jun 2, 2021

mtl1979 commented Jun 4, 2021

nmoinvaz commented Jun 4, 2021

mtl1979 commented Jun 4, 2021

Dead2 commented Jun 8, 2021

nmoinvaz commented Jun 8, 2021

mtl1979 commented Jun 10, 2021

nmoinvaz commented Jun 11, 2021

nmoinvaz commented Jun 11, 2021 • edited

mtl1979 left a comment

Choose a reason for hiding this comment

mtl1979 commented Jun 11, 2021

Dead2 commented Jun 11, 2021

mtl1979 commented Jun 11, 2021

Dead2 commented Jun 11, 2021

mtl1979 commented Jun 11, 2021

Dead2 commented Jun 11, 2021

Baseline 5cfb7f1

PR #982 eb212936dd6da4222f566ea82f8ad48665aa614c

nmoinvaz commented Jun 12, 2021 • edited

mtl1979 commented Jun 12, 2021

mtl1979 commented Jun 12, 2021

nmoinvaz commented Jun 12, 2021

mtl1979 commented Jun 12, 2021

mtl1979 commented Jun 1, 2021 •

edited

codecov bot commented Jun 1, 2021 •

edited

nmoinvaz commented Jun 2, 2021 •

edited

mtl1979 commented Jun 2, 2021 •

edited

nmoinvaz commented Jun 2, 2021 •

edited

nmoinvaz commented Jun 2, 2021 •

edited

nmoinvaz commented Jun 11, 2021 •

edited

Baseline `5cfb7f1`

nmoinvaz commented Jun 12, 2021 •

edited