bugCompact Disc Input and Control Library - Bugs: bug #39373, wrong iso-read >4G .iso due to...

 
 

bug #39373: wrong iso-read >4G .iso due to 32bit offset when should 64bit

Submitted by:  Leo Baschy <infrastrug>
Submitted on:  Sun 30 Jun 2013 09:23:28 PM UTC  
 
Category: run-time errorSeverity: 5 - Average
Item Group: iso-readStatus: Fixed
Privacy: PublicAssigned to: Rocky Bernstein <rocky>
Open/Closed: Closed

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sat 28 Sep 2013 10:37:16 AM UTC, comment #15:

Hereby attaching a iso-read-test.sh that has been tested both in Enterprise Linux and Ubuntu.

Previous version in Ubuntu wasn't working right because of "which" command differences.

Also attached a new .tar.gz of an .iso (4KB for a 1MB .iso) ... if provided then is expanded and used automatically and the script for a 1M .iso doesn't depend on genisoimage or mkisofs. Must match script (e.g. file names in it) or it is a mess. Caching always comes with that risk.

(file #29250, file #29251)

Leo Baschy <infrastrug>
Sat 28 Sep 2013 09:00:12 AM UTC, comment #14:

To be clear, the mystery problem I mentioned last couple posts on this topic has become a bug of its own 40130.

Leo Baschy <infrastrug>
Sat 28 Sep 2013 01:30:56 AM UTC, comment #13:

Should have mentioned, the iso-read problems I saw today in Ubuntu 64 bit on that one not so large .iso file looked more like that other bug, 39354 than this one 39373. Perplexing.

I only mentioned it to correctly report any issues I see, as much as I wish there weren't any and this could become 0.91.

Leo Baschy <infrastrug>
Sat 28 Sep 2013 01:01:42 AM UTC, comment #12:

First I'd like to report a successful test on Ubuntu version 13.04 64 bit against a 5G .iso file, and against a 1M .iso file, i.e. passing iso-read-test.sh.

When running make check, however, I do notice both on SL 6.4 32 bit and Ubuntu 13.04 64 bit the following:

While I had some major progress with iso-read from git HEAD now working better in Ubuntu ...

I am not ready to say all its problems are fixed. Test files passed, one real use of a large .iso file passed, and then one real use of a smaller .iso file failed. To early to say what it has been. I should report when sure one way or the other.

Leo Baschy <infrastrug>
Fri 27 Sep 2013 01:21:30 AM UTC, comment #11:

Ok. Commit 9c9f9d0 has this. Please double check.

Rocky Bernstein <rocky>
Project AdministratorIn charge of this item.
Thu 26 Sep 2013 07:10:17 AM UTC, comment #10:

First line of iso-read-test.sh should be

or else that test will not run in Ubuntu.

Leo Baschy <infrastrug>
Wed 18 Sep 2013 12:12:21 AM UTC, comment #9:

Regarding tests for this bug 39373 and 39354.

Ran into problems (in another project) with genisoimage 1.1.11 crashing in Ubuntu 12.04 LTS and found out about issues around genisoimage and mkisofs.

Hence to avoid libcdio tests from failing on systems where genisoimage or mkisofs have problems (unrelated to libcdio) I have modified iso-read-test.sh so in case there isn't an .iso file but there is an .iso.tar.gz file it now extracts with tar instead of making anew from a directory.

Then if the needed .iso.tar.gz files (made on a first run) are in version control, then no dependency on genisoimage or mkisofs (working or not) exists.

I am attaching a new iso-read-test.sh and a 1 megabyte and a 5000 megabytes .iso.tar.gz, obviously compressed.

(file #29166, file #29167)

Leo Baschy <infrastrug>
Sun 15 Sep 2013 11:28:36 PM UTC, comment #8:

Passes my visual verification of source code and testing.

That is in 32-bit Enterprise Linux 6.4.

Leo Baschy <infrastrug>
Sun 15 Sep 2013 10:39:27 PM UTC, comment #7:

Attached patch applied. Please try with commit ed2c9d1 . Thanks.

Rocky Bernstein <rocky>
Project AdministratorIn charge of this item.
Sun 15 Sep 2013 04:26:08 PM UTC, comment #6:

The email wrapped the diff, hence something got confused and the commit lost adding the p_iso->i_datastart.

The only change against the original code is supposed to be the type cast

for a total assignment calculation of

I should have attached a .diff file instead of just pasting it into the comment. Luckily you requested a check after applying.

Leo Baschy <infrastrug>
Sun 15 Sep 2013 10:17:01 AM UTC, comment #5:

Applied in commit c48003d . Please check to see that the patch was applied correctly.

To ensure a regression doesn't take place, a test (run in only in "maintainer mode") should be added.

Rocky Bernstein <rocky>
Project AdministratorIn charge of this item.
Sun 15 Sep 2013 12:52:10 AM UTC, comment #4:

Here is a single line diff to make iso-read-test.sh pass for a 5 gigabytes large .iso file:

I have tested on 32-bit Enterprise Linux 6.4.

For testing invoke attached iso-read-test.sh with 5000 as additional parameter on the command line. Note that will use 5 gigabytes preparation directory and 5 gigabytes for .iso file for total disk use of 10 gigabytes. Time consuming only on first run, reuses existing file on further runs. Note this .iso file compresses very well, into a mere 5 megabytes (1/1000th) size .tar.gz file, look into directory after it ran. It might be possible to construct a test using less disk space, not sure about that.

This was an "easy" fix now, after 39354 had been fixed, Rock Ridge working, since now it was possible to work on HEAD and apparently a lot of switching over to correct use of 64 bits for offset had been done already since 0.81. Except this little oversight.

Leo Baschy <infrastrug>
Wed 14 Aug 2013 12:57:42 AM UTC, comment #3:

Further improved test script. Has been used to bisect 39354.

Given an integer argument of 5000, works for this defect 39373.

What is new:

  • exits with 0 on success, 1 on failure
  • takes two optional arguments in any order
  • one optional argument if an integer is used as size in megabytes of .iso file it makes, default is 1 for 1MB, and to get 5GB .iso file give it 5000
  • by default only uses 3 megabytes of disk space, no longer gigabytes
  • other optional argument is what iso-read to use, default is plain iso-read, but for example if using with git bisect may make sense to pass src/iso-read

As written here has been run in Enterprise Linux 6.4 and Mac OS X 10.8.

Leo Baschy <infrastrug>
Mon 29 Jul 2013 07:46:56 PM UTC, comment #2:

Here is a better test script for this bug and for bug 39354.

Runs on Mac (tested this version) and on Linux (should run).

Makes two .iso, one 5G large, one 3G large.

Reads back files to verify whether iso-read works correctly.

On repeated runs avoids making .iso again, uses .iso made on first run.

Leo Baschy <infrastrug>
Tue 02 Jul 2013 02:57:35 AM UTC, comment #1:

This test script makes .iso 5G large, uses 10G or 15G while making it, then tries iso-read. Watch for using either mkisofs or genisoimage on your system.

Leo Baschy <infrastrug>
Sun 30 Jun 2013 09:23:28 PM UTC, original submission:

Problem discussed 2010, 2011 at http://lists.gnu.org/archive/html/libcdio-help/2011-12/msg00001.html still a handicap for NrvrCommander.

Current work done with 0.81 because of indepedent problem in 0.90 in bug 39354.

The current test case is seeing nonsense read (4G off in .iso file) by

It does matter the downloaded .iso file is >4G such as http://ftp.scientificlinux.org/linux/scientific/6.4/x86_64/iso/SL-64-x86_64-2013-03-18-Install-DVD.iso

Working example for a slightly smaller .iso <4G, happens to other .iso of same distro, you get a nicely read file

I have dug into it and have "fixed" some issues but haven't succeeded at completing something that works.

First fix is in iso9660_fs.c in iso9660_seek_read_framesize to make i_byte_offset 64 bit, for which I happened to use

because it seems is should be independent of platform it runs on, this is related to .iso files. Either way, as long as it isn't 32 bit.

This library is supposed to run in 32 bit and 64 bit Linux. We have seen problems on 64 bit machines but I have chosen to debug on a 32 bit VM.

Second fix is in same iso9660_fs.c in same iso9660_seek_read_framesize where it multiplies start * p_iso->framesize to first type cast to 64 bit before multiplying like so

Thirdly many functions like cdio_stream_seek must be fixed to allow offset of 64 bit. That includes in _cdio_stream.h and _cdio_stream.c the function cdio_stream_seek parameter i_offset and function cdio_stream_getpos parameter *i_offset and the return value and typedef cdio_data_seek parameter offset.

This also includes in file _cdio_stream.c in struct _CdioDataSource the position must be 64 bit.

One more in _cdio_stdio.c for function parameter i_offset.

I cannot get this right so far, therefore I am filing this bug report so maybe someone else can look too.

Main idea is: Offset into a file (an .iso file) to position can be 64 bit. Remember to cast to 64 bit before multiplying 32 bit or smaller. Everywhere there is a byte offset (also called a position) use 64 bit.

The example .iso are fine for making a fix. If needed I (anyone) can make a special .iso that compresses to close to nothing but expands to >4G for testing purposes in a test suite.

Leo Baschy <infrastrug>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #30554:  iso-read-test.sh added by infrastrug (7kB - application/x-shellscript - without carriage return characters, just line feed, so it runs in RHEL 7 beta)
file #29250:  iso-read-test.sh added by infrastrug (7kB - application/octet-stream)
file #29251:  exampleIso1M.iso.tar.gz added by infrastrug (4kB - application/x-gzip)
file #29166:  iso-read-test.sh added by infrastrug (7kB - application/x-sh)
file #29167:  exampleIso1M.iso.tar.gz added by infrastrug (4kB - application/x-gzip)
file #29145:  iso9660_fs_fix_fix.diff added by infrastrug (590B - application/octet-stream - diff for fix of fix, to avoid more errors in transfer)
file #28837:  iso-read-test.sh added by infrastrug (6kB - application/octet-stream - generally useable iso-read-test.sh script)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by rocky (Posted a comment)
  • -unavailable- added by infrastrug (Submitted the item)
  • -unavailable- added by infrastrug
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 04 Jun 2014 07:10:23 PM UTCrockyOpen/ClosedOpen=>Closed
    Sat 15 Feb 2014 10:01:50 AM UTCinfrastrugAttached File-=>Added iso-read-test.sh, #30554
    Thu 10 Oct 2013 12:55:39 AM UTCrockyStatusNone=>Fixed
    Sat 28 Sep 2013 10:37:16 AM UTCinfrastrugAttached File-=>Added iso-read-test.sh, #29250
      Attached File-=>Added exampleIso1M.iso.tar.gz, #29251
    Fri 27 Sep 2013 01:21:30 AM UTCrockyAssigned toNone=>rocky
    Wed 18 Sep 2013 12:12:21 AM UTCinfrastrugAttached File-=>Added iso-read-test.sh, #29166
      Attached File-=>Added exampleIso1M.iso.tar.gz, #29167
    Sun 15 Sep 2013 06:49:50 PM UTCinfrastrugAttached File-=>Added iso9660_fs_fix_fix.diff, #29145
    Wed 14 Aug 2013 01:17:10 AM UTCinfrastrugAttached File-=>Added iso-read-test.sh, #28837
    Sun 30 Jun 2013 09:23:28 PM UTCinfrastrugCarbon-Copy-=>Added infrastrug

    Back to the top


    Powered by Savane 3.1-cleanup