iso9660 file system driver buffer overflows

[beos_zealot]

Spotted while building rev45325 gcc4 image on Haiku gcc4 rev45316. Build log showed errors "array subscript is above array bounds".

Patch attached.

[umccullough]

It just so happens all of these are detected by coverity as well, so whomever applies the patches, please consult coverity and mark those issues as fixed.

for reference: CID 601359

[leavengood]

Thanks for the patch but why the heck is the same magic number repeated in 9 places in the original code? Surely even a #define would be preferable, or better yet a static const.

[beos_zealot]

maybe i am wrong, but i guess that at first in iso9660.h header file was created iso9660 primary volume descriptor structure(struct iso9660_volume) from iso9660 specifications, then "magic numbers" were copied from header to source file and developer forgot to edit them. i think it's just a human error, it happens to everyone once in a while...

[beos_zealot]

interesting fact - bug comes from 1999 :) from BeOS sample code (​http://haikuware.com/directory/start-download/development/sample-code/beos-r5-sample-code).

source file sample-code\add-ons\iso9660\iso.c has the same bug.

[leavengood]

That code is just terrible with all the hard-coded numbers. At least for this patch could you use 'sizeof() - 1' instead of 37?

[beos_zealot]

As you recommended, i changed hard-coded '37' to 'sizeof() - 1'.

Modified patch attached.

[pdziepak]

Wouldn't it be neater if the value of sizeof(anArrayWeWantToWriteTo) - 1 was firstly assigned to some const size_t constant and then used three times: zeroing the last byte of the array, setting length for strncpy() and increasing buffer pointer? The compiler will optimize it anyway and we won't end up with six volume->copyright in four consecutive lines.

[beos_zealot]

You are absolutely right, i am a bit hurried with patch modifications, so now i am better go to sleep (it's 23:30 after all) - tommorow with clear head i'll try to make patch as it should be...

[jscipione]

I'd just apply the original patch unless beos_zealot wants to refactor the entire function. The original patch makes a very small change which directly targets the bug in the manner that the code was originally written.

It's not the bug reporters job to fix the design problems of the original code, just fix this particular bug and the original patch does this.

[axeld]

It surely doesn't hurt to improve the code quality - I would prefer to have two different commits, though. This can be nicely done using git format-patch. So if beos_zealot is willing to do that, it would be very much appreciated.

I agree with John, though, that the patch as is would be good enough. However, I would also prefer if the additional changes would be committed in a single revision.

[beos_zealot]

Yes, i'm willing to do that and as promised today. For now i'm refactored code in "InitVolDesc" function and in few related places.

Please, review patch (find newly introduced bugs :))

[mmadia]

hi. Thanks for sticking with this and refactoring iso9660. Unfortunately, your new patch does not apply cleanly on the current master (hrev45330). Is one of the patches missing?

/use-the-source/haiku> git apply --check 0002-iso9660-fs-driver-code-refactoring-with-bugfix.patch 
error: patch failed: src/add-ons/kernel/file_systems/iso9660/iso9660.cpp:163
error: src/add-ons/kernel/file_systems/iso9660/iso9660.cpp: patch does not apply
/use-the-source/haiku> 

[beos_zealot]

As recommended by Axel patch done in two commits:

1. At first you have to apply actual bugfix patch:

0001-iso9660-file-system-driver-buffer-overflows-bugfix.patch​

2. Second patch - code refactoring (a name of patch a bit confusing as it should'nt have part "-with-bugfix" in it's name):

0002-iso9660-fs-driver-code-refactoring-with-bugfix.patch​

[beos_zealot]

iso9660 fs driver bugfix and code refactoring (slightly updated) as one commit in one patch:

0001-iso9660-fs-driver-bugfix-code-refactoring.patch

[mmadia]

Thanks for the new patch. (Obviously, I hadn't realized that it was intended to apply the first, then the previous one).

To note, this patch applies cleanly (barring some whitespace warnings in src/add-ons/kernel/file_systems/iso9660/iso9660.cpp) and compiles fine with gcc2 and gcc4.

[leavengood]

Please commit this patch mmadia.

[mmadia]

Applied in hrev45361. Thanks!