Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZSTD_NO_FORWARD_PROGRESS_MAX case should give more explicit error #3454

Closed
lovrop opened this issue Jan 25, 2023 · 1 comment · Fixed by #3455
Closed

ZSTD_NO_FORWARD_PROGRESS_MAX case should give more explicit error #3454

lovrop opened this issue Jan 25, 2023 · 1 comment · Fixed by #3455
Assignees

Comments

@lovrop
Copy link

lovrop commented Jan 25, 2023

Hello.

When using zstd in a streaming context (file which gets extended at arbitrary times) it seems reasonable to structure a decompression loop like so:

size_t streaming_decompress(State* state, char* out, size_t out_size) {
    ZSTD_outBuffer out = {out, out_size, 0};
    while (1) {
      ZSTD_inBuffer in = {state->compressed_buf, state->compressed_buf_size, state->compressed_buf_pos};
      size_t rv = ZSTD_decompressStream(zds, &out, &in);
      if (ZSTD_isError(rv)) {
        // complain
      }
      if (out.pos == out.size) {
        // exhausted output buffer, record progress through input buffer for next call
        state->compressed_buf_pos = in.pos;
        break;
      }
      // exhausted input buffer, try to read more data from file
      if (try_refill_compressed_buf(state)) {
        // got more compressed data, loop around
      } else {
        // no more data available at the moment
        break;
      }
    }
    return out.pos;
}

If no new data has come in for a while, each invocation of this function calls into libzstd (no-op) then tries to refill (also no-op). On the 16th attempt this fails to the ZSTD_NO_FORWARD_PROGRESS_MAX check. This took me a while to track down because:

  1. A check like that was completely unexpected, resulting in some incredulity as to why this call would work some of the time then fail with the exact same parameters. I was going to post a minimal repro as a bug report when a colleague had to point out the existence of this check.
  2. The error message Src size is incorrect was not helpful in figuring out the root cause.

I won't discuss the merit of the check beyond the above as you've obviously intentionally put it in, but my suggestion is to provide more explicit error messages, for example No forward progress for a while due to src.pos == src.size and similar for dst.

Thank you!

@Cyan4973
Copy link
Contributor

Yes, a more accurate error code will likely be helpful for this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants