Opened 7 years ago

Closed 7 years ago

#9486 closed bug (fixed)

iso9660 file system driver buffer overflows

Reported by: beos_zealot Owned by: nobody
Priority: normal Milestone: R1
Component: File Systems/ISO 9660 Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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

Patch attached.

Change History (23)

comment:1 by beos_zealot, 7 years ago

Has a Patch: set

comment:2 by beos_zealot, 7 years ago

Summary: iso9660 file system driver buffer overflows bugfixiso9660 file system driver buffer overflows

comment:3 by umccullough, 7 years ago

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

Last edited 7 years ago by umccullough (previous) (diff)

comment:4 by leavengood, 7 years ago

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.

comment:5 by beos_zealot, 7 years ago

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

comment:6 by beos_zealot, 7 years ago

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.

comment:7 by leavengood, 7 years ago

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

Last edited 7 years ago by leavengood (previous) (diff)

comment:8 by beos_zealot, 7 years ago

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

Modified patch attached.

comment:9 by pdziepak, 7 years ago

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.

comment:10 by beos_zealot, 7 years ago

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

Last edited 7 years ago by beos_zealot (previous) (diff)

comment:11 by jscipione, 7 years ago

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.

comment:12 by axeld, 7 years ago

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.

comment:13 by beos_zealot, 7 years ago

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 :))

comment:14 by mmadia, 7 years ago

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> 

comment:15 by beos_zealot, 7 years ago

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​

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

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

Last edited 7 years ago by beos_zealot (previous) (diff)

comment:16 by beos_zealot, 7 years ago

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

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

comment:17 by mmadia, 7 years ago

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.

comment:18 by leavengood, 7 years ago

Please commit this patch mmadia.

comment:19 by mmadia, 7 years ago

Resolution: fixed
Status: newclosed

Applied in hrev45361. Thanks!

Note: See TracTickets for help on using tickets.