zsync crashes with SIGSEGV when updating dvds

StacktraceTop:rcksum_submit_blocks (z=0x1516d10,
zsync_receive_data (zr=0x1517fa0,
fetch_remaining_blocks_http (
fetch_remaining_blocks (zs=0x1516c20)
main (argc=<value optimized out>,

The last zsync merge did the following:

@@ -1000,23 +1006,11 @@

             /* Otherwise, we're reading the MIME headers for this part until we get \r\n alone */
             for (; buf[0] != '\r' && buf[0] != '\n' && buf[0] != '\0';) {
- off_t from, to;
-
- /* Get next header */
- if (!rfgets(buf, sizeof(buf), rf))
- return 0;
- buflwr(buf); /* HTTP headers are case insensitive */
-
- /* We're looking for the Content-Range: header, to tell us how
- * many bytes and what part of the target file they represent.
- */
- if (2 ==
- sscanf(buf,
- "content-range: bytes " OFF_T_PF "-" OFF_T_PF "/",
- &from, &to)) {
- rf->offset = from;
- rf->block_left = to - from + 1;
- gotr = 1;
+ int from, to;
+ if (!rfgets(buf,sizeof(buf),rf)) return 0;
+ buflwr(buf);
+ if (2 == sscanf(buf,"content-range: bytes %d-%d/",&from,&to)) {
+ rf->offset = from - global_offset; rf->block_left = to - from + 1; gotr = 1;
                 }
             }

which changes from and to from off_t to ints; unfortunately, the dvds that I'm attempting download via zsync are larger than 2GB, so from and to suffer from signed int overflows in this case. I've reverted the code in this section mostly back to the way upstream had it, keeping the 'rf->offset = from - global_offset;' bit, since that's what the actual intended ubuntu difference (to support the undocumented -O global offset argument, apparently) consists of. I've pushed this fix to lp:~sbeattie/ubuntu/karmic/zsync/zsync-fixups and built a package for testing in my ppa at https://launchpad.net/~sbeattie/+archive/ppa/. I've been using this package for a few hours and I'm able to download a daily ubuntu dvd that with the 0.6-1ubuntu1 version consistently SEGV's.

Is there any upstream bug about this where the problem is discussed too?

On Thu, Sep 03, 2009 at 05:13:44AM -0000, Daniel Holbach wrote:
> Is there any upstream bug about this where the problem is discussed too?

Poking around http://zsync.moria.org.uk/ , no, it
doesn't appear that there is, nor is there in debian's
BTS (it looks like the upstream author is also the debian
maintainer). However, if you look at rev 5 in ubuntu's history
(http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/karmic/zsync/karmic/revision/5),
you'll see in http.c lines 635-640 (in the committed version) where
global_offset is being added by ubuntu.

You can get upstream's bzr tree at http://zsync.moria.org.uk/zsync/
though it's not web browsable (it's not big, there's only 92 revisions).
Revision 39 changes the types of "from" and "to" in question (c/http.c's
diff chunk annotated with "@@ -676,25 +683,28 @@") from an int to off_t
(along with a lot of other similar changes); unfortunately, the commit
message is the unhelpful:

  $ bzr log -c 39
  ------------------------------------------------------------
  revno: 39
  committer: Colin Phipps <email address hidden>
  branch nick: zsync
  timestamp: Sat 2006-08-05 17:43:19 +0100
  message:
    Fix off_t and size_t format strings.
    tabs -> spaces.
    stdint.h in zmap.c, for MacOS X.

which doesn't explain the why. Revision 39 corresponds to version 4.3 =>
5.0 in the upstream releases.

I've attached the full diff for rev 39 for your perusal, though of
course, looking at upstream's bzr tree is recommended as well.

Another way to fix this would be to drop the undocumented -O
(global_offset) in the first place; this would significantly minimize
our difference from upstream. I can't find any reference as to why we
added support for it. If that's the preferred solution, I can generate
that patch as well.

--
Steve Beattie
<email address hidden>
http://NxNW.org/~steve/

I don't object to the change at all and it obviously seems to make things work again. Maybe somebody else can just upload it if they feel comfortable doing so, I personally would just prefer to see this discussed with upstream somehow.

This bug was fixed in the package zsync - 0.6-1ubuntu2

---------------
zsync (0.6-1ubuntu2) karmic; urgency=low

  * http.c: fixup offsets that are larger than signed ints (LP: #420931)

 -- Steve Beattie <email address hidden> Mon, 31 Aug 2009 02:14:24 -0700

I've put together a backport for an SRU; attached is the debdiff for hardy. I've also built test packages based off of the backported patch for testing in my zsync ppa at https://launchpad.net/~sbeattie/+archive/zsync/. I wasn't sure the rest of the zsync 0.5 codebase was 64bit clean, but based on local testing, zsync 0.5 + the backported patch successfully downloads full dvd images where the released version would segv, and continues to download smaller sized files.

Intrepid debdiff.

Jaunty debdiff.

Oh, bah, I realized I didn't target the debdiffs to -proposed; attaching
new versions.

--
Steve Beattie
<email address hidden>
http://NxNW.org/~steve/

I've uploaded the package to hardy, intrepid and jaunty -proposed.

This is still needing approval from motu-sru before it can be accepted.

ACK from MOTU-SRU for all 3 debdiffs; Thanks Steve!

Accepted into jaunty-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Accepted into dest=intrepid-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Accepted into dest=hardy-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Hardy server works :)

No segfaults no temp files left behind.

2.6.24-24-server #1 SMP Fri Sep 18 16:47:05 UTC 2009 x86_64 GNU/Linux

This bug was fixed in the package zsync - 0.5-1ubuntu3.8.04.1

---------------
zsync (0.5-1ubuntu3.8.04.1) hardy-proposed; urgency=low

  * http.c: fixup offsets that are larger than signed ints (LP: #420931)

 -- Steve Beattie <email address hidden> Sat, 19 Sep 2009 18:30:36 -0700

Dave, thanks for testing!

Leaving as verification-needed for intrepid/jaunty.

The bug is gone on jaunty 64bit.
I updated the 4GB karmic dvd. ctrl-c'ing and resuming at 60% and again at 70% to try to trigger the bug.

zsync on Intrepid 32bit does not work. See Bug #412413. It refuses to go beyond 2GB. No segfault, just aborting.

Wolfgang, thanks so much for testing. I asked Dave Morley on IRC and he
confirmed that his system that he tested the Hardy version on is amd64.

I was afraid when I backported the fix that the rest of zsync 0.5
wasn't 64bit clean, as a number of the changes between 0.5 and 0.6
(karmic's version) looked like they addressed such issues. They are
also too invasive IMO for an SRU, but I'll prepare a backport to
mitigate bug 412413.

Martin, I think it's still worth keeping this SRU, as it improves the
situation for amd64 users, and doesn't appear to create any regressions
for i386 users.

This bug was fixed in the package zsync - 0.5-1ubuntu3.9.04.1

---------------
zsync (0.5-1ubuntu3.9.04.1) jaunty-proposed; urgency=low

  * http.c: fixup offsets that are larger than signed ints (LP: #420931)

 -- Steve Beattie <email address hidden> Sat, 19 Sep 2009 18:30:36 -0700

Keeping v-needed for intrepid update.

Can anyone test this on intrepid?

This intrepid-proposed SRU has not been verified in the last three months or longer. Intrepid will go out of support in less than two months, so it is not worth pursuing this SRU any further.

I removed the intrepid-proposed version from the archive.