Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0b3bb1d) 69.90% compared to head (b0607c3) 83.27%.

@@             Coverage Diff              @@
##           develop    #1602       +/-   ##
============================================
+ Coverage    69.90%   83.27%   +13.37%     
============================================
  Files          116      133       +17     
  Lines         9479    10880     +1401     
  Branches      2341     2809      +468     
============================================
+ Hits          6626     9060     +2434     
+ Misses        1777     1117      -660     
+ Partials      1076      703      -373     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OK, I think this will work. It's maybe a bit hackier than I'd like but I'm pretty sure correctness isn't an issue. This should catch the corner case we were seeing. The main difference is I think there were unconditional checksums being applied in earlier versions in a lot of places. I don't see a clean delineation where we clearly aren't applying the checksum but should be, though.

Hmm, I have half a mind to write the buffer being fed through that library's test suite and put it into ours.

Not sure what's up with the clang reduced memory failure.

Ahh, it does look like it reduces the window size to 8k, so perhaps this is making us hit a different corner case. It also has dawned on me that the checksum will always be the initial CRC32 value until the final fold so that bit might need to be reworked.

I reproduced the reduced mem failure, I'll look into it tonight.

Ok I think I got it. Still a little hackier than I'd like (I'd be much more thrilled about catching the set of conditions that paints us in this corner) but barring better suggestions or proof that this is not catching all of the corner cases, I say it's ready to merge.

Wish we did have a CI test for this, so we could make sure that this is not a problem for all the other alternative code-paths as well. I don't think any other arch based their code on crc32_fold_pclmulqdq etc and inherited the problem, did they?

The problem isn't specific to crcs, it's specific to inflate. There's a potential encoding through the compression state machine that arrives at the final check instruction without the current window getting run through the checksum routines.

This seems to catch those corner cases, as when it happens the checksum is the initial unaltered checksum, be that Adler or crc.

As far as something that catches this, I think integrating libgit2's tests into our CI wouldn't be a bad approach.

I am testing using minideflate and comparing to libgit2. The put variable sent to inf_chksum is different.

minideflate using a next position in write buffer for each call to inflate(), where as libgit2 starts from scratch from the buffer, so put is evaluated correctly.

Here is my test data:
000000A2CD78EBA0-inflate.gz

minideflate -d -k -s 4 -r 25 -w 15 -t 8192 000000A2CD78EBA0-inflate.gz

And change minideflate:

            err = PREFIX(inflate)(&d_stream, flush);
            if (err == Z_BUF_ERROR && read_buf_size != BUFSIZE) {
                err = Z_OK;
            }

So essentially, it only works because in libgit2 they are passing the same buffer pointer of the previous inflate() to the the next call to inflate(). I still don't think we should commit this until we add a unit test.

This also seems to work although it can probably be better some how:

  inf_leave:
    RESTORE();
    if (INFLATE_NEED_UPDATEWINDOW(strm) &&
            (state->wsize || (out != strm->avail_out && state->mode < BAD &&
                 (state->mode < CHECK || flush != Z_FINISH)))) {
        /* update sliding window with respective checksum if not in "raw" mode */
        if (updatewindow(strm, strm->next_out, out - strm->avail_out, state->wrap & 4)) {
            state->mode = MEM;
            return Z_MEM_ERROR;
        }
-   }
+   } else if (!state->wsize && flush == Z_FINISH) {
+       inf_chksum(strm, put - (out - strm->avail_out), (out - strm->avail_out));
+   }

Vars: state->wsize = 0, flush = Z_FINISH, state->mode = CHECK

In this use case Z_FINISH is being used which according to the docs:

Normally, inflate might already have a window if it had been previously called with another flush type. But in libgit2 case it is very short length and calling it with Z_FINISH, a window doesn't get created or used.

This also seems to work although it can probably be better some how:

  inf_leave:
    RESTORE();
    if (INFLATE_NEED_UPDATEWINDOW(strm) &&
            (state->wsize || (out != strm->avail_out && state->mode < BAD &&
                 (state->mode < CHECK || flush != Z_FINISH)))) {
        /* update sliding window with respective checksum if not in "raw" mode */
        if (updatewindow(strm, strm->next_out, out - strm->avail_out, state->wrap & 4)) {
            state->mode = MEM;
            return Z_MEM_ERROR;
        }
-   }
+   } else if (!state->wsize && flush == Z_FINISH) {
+       inf_chksum(strm, put - (out - strm->avail_out), (out - strm->avail_out));
+   }

Vars: state->wsize = 0, flush = Z_FINISH, state->mode = CHECK

In this use case Z_FINISH is being used which according to the docs:

Normally, inflate might already have a window if it had been previously called with another flush type. But in libgit2 case it is very short length and calling it with Z_FINISH, a window doesn't get created or used.

That solution is a lot cleaner assuming it captures every instance of this. I was hoping to find a solution more like that but couldn't bring myself to find the right spot and conditions to inject that extra forced checksum. So the right conditions are specifically if the user has specified Z_FINISH, bypassing the window creation entirely and effectively it's moved to the end stage "check" state

I like @nmoinvaz's solution much better, way less of a hack, way fewer lines, fewer comparisons. @nmoinvaz can you file a PR with your minideflate changes and that simple reproducer as a test case?

Yes, I can create a PR with my minideflate use case. I looked putting my suggested logic in CHECK mode instead, but CHECK mode bails early in NEEDSBITS with Z_BUF_ERROR. So I think at the bottom of the function like I originally suggested is probably the best place for the logic.

LGTM, now we just need to rebase after #1603 is merged.

Rebase please 👍

Guessing we can probably ignore that failure to upload issue?

Rerolling the dice anyway for that green check mark.

Looking good, all tests passing. I will probably roll out a hotfix release with this within a day or two, unless something crops up.