zsync crashes with SIGSEGV when updating dvds

Bug #420931 reported by Steve Beattie on 2009-08-29
20
This bug affects 1 person
Affects Status Importance Assigned to Milestone
zsync (Ubuntu)
Medium
Unassigned
Hardy
Medium
Unassigned
Intrepid
Medium
Unassigned
Jaunty
Medium
Unassigned

Bug Description

Binary package hint: zsync

Was zsync'ing http://cdimages.ubuntu.com//kubuntu/dvd/20090828/karmic-dvd-i386.iso

ProblemType: Crash
Architecture: amd64
Date: Fri Aug 28 21:48:36 2009
Dependencies:
 findutils 4.4.2-1
 gcc-4.4-base 4.4.1-3ubuntu1
 libc6 2.10.1-0ubuntu7
 libgcc1 1:4.4.1-3ubuntu1
DistroRelease: Ubuntu 9.10
ExecutablePath: /usr/bin/zsync
Package: zsync 0.6-1ubuntu1
ProcCmdline: /usr/bin/zsync -k kubuntu/.karmic-dvd-i386.iso.zsync -o kubuntu/karmic-dvd-i386.iso http://cdimages.ubuntu.com//kubuntu/dvd/current/karmic-dvd-i386.iso.zsync
ProcEnviron:
 PATH=(custom, user)
 LANG=en_US.UTF-8
 SHELL=bash
ProcVersionSignature: Ubuntu 2.6.31-7.27-server
SegvAnalysis:
 Segfault happened at: 0x4098a5 <fflush@plt+31821>: repz cmpsb %es:(%rdi),%ds:(%rsi)
 PC (0x004098a5) ok
 source "%es:(%rdi)" (0x7f09712e9d1c) not located in a known VMA region (needed readable region)!
 destination "%ds:(%rsi)" (0x7fffc9b807c0) ok
SegvReason: reading unknown VMA
Signal: 11
SourcePackage: zsync
StacktraceTop:
 ?? ()
 ?? ()
 ?? ()
 ?? ()
 ?? ()
Title: zsync crashed with SIGSEGV
Uname: Linux 2.6.31-7-server x86_64
UserGroups: adm admin cdrom dialout kvm lpadmin plugdev sambashare

TESTCASE:
zsync a dvd from cdimages.ubuntu.com, e.g.:
  $ /usr/bin/zsync -k .karmic-dvd-i386.iso.zsync -o karmic-dvd-i386.iso http://cdimage.ubuntu.com//edubuntu/dvd/current/karmic-dvd-i386.iso.zsync
You may need to kill the download partway through (after 2GB of data has been downloaded) and then resume to trigger the segv.

Steve Beattie (sbeattie) wrote :
visibility: private → public

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>,

Changed in zsync (Ubuntu):
importance: Undecided → Medium
tags: removed: need-amd64-retrace

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.

Changed in zsync (Ubuntu):
status: New → In Progress
summary: - zsync crashed with SIGSEGV
+ zsync crashes with SIGSEGV when updating dvds
Steve Beattie (sbeattie) on 2009-09-01
Changed in zsync (Ubuntu):
status: In Progress → Fix Committed
Daniel Holbach (dholbach) wrote :

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/

Daniel Holbach (dholbach) wrote :

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.

Launchpad Janitor (janitor) wrote :

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

Changed in zsync (Ubuntu):
status: Fix Committed → Fix Released
Steve Beattie (sbeattie) on 2009-09-23
description: updated
Steve Beattie (sbeattie) wrote :

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.

Steve Beattie (sbeattie) wrote :

Intrepid debdiff.

Steve Beattie (sbeattie) wrote :

Jaunty debdiff.

Changed in zsync (Ubuntu Hardy):
status: New → In Progress
Steve Beattie (sbeattie) wrote :

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

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

Changed in zsync (Ubuntu Intrepid):
status: New → In Progress
Changed in zsync (Ubuntu Jaunty):
status: New → In Progress
Changed in zsync (Ubuntu Hardy):
importance: Undecided → Medium
Changed in zsync (Ubuntu Intrepid):
importance: Undecided → Medium
Changed in zsync (Ubuntu Jaunty):
importance: Undecided → Medium
Brian Murray (brian-murray) wrote :

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

Scott Kitterman (kitterman) wrote :

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

John Dong (jdong) wrote :

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

Scott Kitterman (kitterman) wrote :

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!

Changed in zsync (Ubuntu Jaunty):
status: In Progress → Fix Committed
tags: added: verification-needed
Scott Kitterman (kitterman) wrote :

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!

Scott Kitterman (kitterman) wrote :

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!

Changed in zsync (Ubuntu Hardy):
status: In Progress → Fix Committed
Changed in zsync (Ubuntu Intrepid):
status: In Progress → Fix Committed
Dave Morley (davmor2) wrote :

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

Launchpad Janitor (janitor) wrote :

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

Changed in zsync (Ubuntu Hardy):
status: Fix Committed → Fix Released
Martin Pitt (pitti) wrote :

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.

Launchpad Janitor (janitor) wrote :

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

Changed in zsync (Ubuntu Jaunty):
status: Fix Committed → Fix Released
Martin Pitt (pitti) wrote :

Keeping v-needed for intrepid update.

Martin Pitt (pitti) wrote :

Can anyone test this on intrepid?

Martin Pitt (pitti) wrote :

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.

Changed in zsync (Ubuntu Intrepid):
status: Fix Committed → Won't Fix
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers