Note: There are a few cases of duplicates in user autocompletion which are being worked on.

GIF2.cpp: do_lzw() tries to access memory out of bounds [@ do_lzw]

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: jhp (no longer active), Assigned: dveditz)

Tracking

(4 keywords)

Trunk
PowerPC
Mac OS X
fixed1.8.0.2, fixed1.8.1, regression, topcrash
Points:
---
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high?] Doesn't affect ff1.0/moz1.7 [tcn-dl], crash signature, URL)

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
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

12 years ago
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.
Attachment #213232 - Flags: review?(jhpedemonte)
(Reporter)

Comment 3

12 years ago
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?
Attachment #213232 - Flags: review?(jhpedemonte) → review+
isn't this a duplicate of bug 243653?

Comment 5

12 years ago
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...
Attachment #213232 - Flags: superreview?(pavlov)

Updated

12 years ago
Attachment #213232 - Flags: superreview?(pavlov) → superreview+
Blocks: 328258

Comment 6

12 years ago
Thanks, who can check this for me?

Comment 7

12 years ago
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).
Flags: blocking1.8.0.2?
(Assignee)

Updated

12 years ago
Group: security
Flags: blocking1.9a1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Whiteboard: [sg:??]
(Assignee)

Comment 8

12 years ago
The memset line was added as part of bug 274391, well after the 1.7/aviary101 branches.
Keywords: regression
Whiteboard: [sg:??] → [sg:??] Doesn't affect ff1.0/moz1.7
(Assignee)

Comment 9

12 years ago
I guess I'll take this to land it.
Assignee: pavlov → dveditz
(Assignee)

Comment 10

12 years ago
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?
Keywords: fixed1.8.0.2, fixed1.8.1
Please provide testcase and guidance on how to verify this fix.
Whiteboard: [sg:??] Doesn't affect ff1.0/moz1.7 → [sg:??] Doesn't affect ff1.0/moz1.7 [tcn-dl]
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.
Keywords: topcrash
Summary: GIF2.cpp: do_lzw() tries to access memory out of bounds → GIF2.cpp: do_lzw() tries to access memory out of bounds [@ do_lzw]
(Assignee)

Comment 13

12 years ago
fixed per comment 10 on March 3
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:??] Doesn't affect ff1.0/moz1.7 [tcn-dl] → [sg:high?] Doesn't affect ff1.0/moz1.7 [tcn-dl]
(Assignee)

Comment 14

11 years ago
*** Bug 317536 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

11 years ago
Group: security

Updated

10 years ago
No longer blocks: 328258
Crash Signature: [@ do_lzw]
You need to log in before you can comment on or make changes to this bug.