Opened 3 years ago

Closed 3 years ago

Last modified 18 months ago

#11824 closed defect (fixed)

tor_zlib_process fails when trying to finish with no input or output bytes.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay, zlib, 025-backport, 026-triaged-0, nickm-patch, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

zlib reports Z_BUF_ERROR when it is out of input or out of output. Currently, we handle that with:

    case Z_BUF_ERROR:
      if (state->stream.avail_in == 0)
        return TOR_ZLIB_OK;
      return TOR_ZLIB_BUF_FULL;

But that's wrong -- if avail_in is 0, but finish is set, then we are trying to finalize the stream, and we really do have something to write. That test should be state->stream.avail_in == 0 && !finish)

Marking this for 0.2.5 even though I am pretty sure this can never actually happen with the way write_to_buf_zlib works: write_to_buf_zlib ensures that we are never trying to write into a completely empty buffer, and zlib says "Z_OK" if you give it even one byte to write into.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by nickm

Keywords: tor-relay zlib added

comment:2 Changed 3 years ago by nickm

Status: newneeds_review

comment:3 Changed 3 years ago by wfn

fwiw, the state->stream.avail_in == 0 && !finish (or state->stream.avail_in == 0 && state->stream.avail_out > 0, I guess?) test makes sense.

comment:4 Changed 3 years ago by nickm

I put this in "bug11824" since I realize that I only gave a verbal description of the change above. It will need a real commit message and changes file before it can get merged.

comment:5 Changed 3 years ago by nickm

Keywords: 025-backport added
Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

Moving to 0.2.6 with possible backport, since the effects of breaking this code would not be fun to try to solve late in a release series.

comment:6 Changed 3 years ago by nickm

Keywords: 026-triaged added

comment:7 Changed 3 years ago by nickm

Keywords: 026-triaged-0 added; 026-triaged removed

comment:8 Changed 3 years ago by nickm

I've written a commit message and a changes file for this. It's only a one-line commit. Review pls? New branch is "bug11824_v2"

comment:9 Changed 3 years ago by nickm

Keywords: nickm-patch added

Apply a nickm-patch keyword to tickets in needs_review in 0.2.6 where I wrote the patch, so I know which ones I can('t) review myself.

comment:10 Changed 3 years ago by wfn

LGTM (from what I recall when I looked at torgzip before, and from looking at the logic now), but my knowledge of zlib is very limited.

Cosmetics:

[...]
output buffer, do not report the the write as successful.

s/the the write/the write/

comment:11 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay; still looks okay for me, so merging.

comment:12 Changed 18 months ago by nickm

Keywords: 2016-bug-retrospective added

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:13 Changed 18 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:14 Changed 18 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

Note: See TracTickets for help on using tickets.