Last Comment Bug 328509 - GIF2.cpp: do_lzw() tries to access memory out of bounds [@ do_lzw]
: GIF2.cpp: do_lzw() tries to access memory out of bounds [@ do_lzw]
Status: RESOLVED FIXED
[sg:high?] Doesn't affect ff1.0/moz1....
: fixed1.8.0.2, fixed1.8.1, regression, topcrash
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
https://bugzilla.mozilla.org/attachme...
: 317536 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-24 12:38 PST by jhp (no longer active)
Modified: 2007-10-01 19:14 PDT (History)
8 users (show)
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
One line patch to fix clearing of struct (786 bytes, patch)
2006-02-26 02:24 PST, Alfred Kayser
jhpedemonte: review+
pavlov: superreview+
Details | Diff | Splinter Review

Description jhp (no longer active) 2006-02-24 12:38:50 PST
Offshoot of bug 328258.

Debugging using the gif attached at bug 328258 (https://bugzilla.mozilla.org/attachment.cgi?id=212852), I noticed that do_lzw() is accessing memory out of bounds of the array.  Specifically, in this code:

      while (code >= clear_code)
      {
        if (code == prefix[code])
          return -1;

        *stackp++ = suffix[code];
        code = prefix[code];

        if (stackp == stack + MAX_BITS)
          return -1;
      }

At one point, when count == 190, the line "code = prefix[code]" sets code to 43690.  The next time through the loop, it tries to access prefix[43690], but prefix is of size 4097.
Comment 1 jhp (no longer active) 2006-02-24 14:59:09 PST
In GIFInit, I changed this line:
  memset(gs, 0, offsetof(gif_struct, prefix));
to:
  memset(gs, 0, sizeof(gif_struct));

With this, I don't see it trying to access invalid memory.  Seems the while loop tries to access an offset of 'prefix' that has never been set up.
Comment 2 Alfred Kayser 2006-02-26 02:24:37 PST
Created attachment 213232 [details] [diff] [review]
One line patch to fix clearing of struct

The offsetof thing was to clear only part of the struct, but is clearly wrong. 
It is saver anyway to clear the whole struct.
Comment 3 jhp (no longer active) 2006-02-26 06:53:20 PST
Comment on attachment 213232 [details] [diff] [review]
One line patch to fix clearing of struct

This is fine.  However, as part of LZW, should the algorithm access an offset of prefix that hasn't previously been set?  With this fix, the result will now be zero.  Is that correct according to the LZW spec?
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2006-02-26 15:49:59 PST
isn't this a duplicate of bug 243653?
Comment 5 Alfred Kayser 2006-02-27 00:39:50 PST
Comment on attachment 213232 [details] [diff] [review]
One line patch to fix clearing of struct

Pavlov, can you take quickly a look at this. This is related to a 'Access Denied' bug...
Comment 6 Alfred Kayser 2006-02-28 01:14:44 PST
Thanks, who can check this for me?
Comment 7 Alfred Kayser 2006-02-28 10:28:09 PST
Correction: 
Previous comment should be:

Thanks, who can check this in the tree(s) for me?

Also requesting to check it into the 1.8 branch for FF 2.0, because this a crasher (and related to bug 328258).
Comment 8 Daniel Veditz [:dveditz] 2006-03-01 14:26:31 PST
The memset line was added as part of bug 274391, well after the 1.7/aviary101 branches.
Comment 9 Daniel Veditz [:dveditz] 2006-03-02 12:50:32 PST
I guess I'll take this to land it.
Comment 10 Daniel Veditz [:dveditz] 2006-03-03 15:53:36 PST
Fix landed on trunk, 1.8 branch, and 1.8.1 branch

What is the security impact here? Looks like it's only reading random memory, right?
Comment 11 Dave Liebreich [:davel] 2006-03-04 13:00:55 PST
Please provide testcase and guidance on how to verify this fix.
Comment 12 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2006-03-10 12:39:24 PST
FWIW, this patch fixed the uninitialized memory reads and free memory reads reported by valgrind on the GIF in the testcase on bug 317536.  That bug should be marked duplicate or depends+fixed when this bug is opened.

Crashes in do_lzw are a topcrash for Fx 1.5.0.1; I suspect this fix fixes them.
Comment 13 Daniel Veditz [:dveditz] 2006-03-28 17:06:57 PST
fixed per comment 10 on March 3
Comment 14 Daniel Veditz [:dveditz] 2006-04-13 16:55:01 PDT
*** Bug 317536 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.