Last Comment Bug 709483 - Off-by-one in dom/base/nsDOMClassInfo.cpp
: Off-by-one in dom/base/nsDOMClassInfo.cpp
Status: RESOLVED FIXED
[asan]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Christian Holler (:decoder)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 664901
  Show dependency treegraph
 
Reported: 2011-12-10 09:14 PST by Christian Holler (:decoder)
Modified: 2011-12-14 22:27 PST (History)
5 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (849 bytes, patch)
2011-12-12 17:51 PST, Christian Holler (:decoder)
bzbarsky: review+
Details | Diff | Splinter Review
Updated patch (849 bytes, patch)
2011-12-14 04:29 PST, Christian Holler (:decoder)
choller: review+
Details | Diff | Splinter Review
Updated patch (1.07 KB, patch)
2011-12-14 04:31 PST, Christian Holler (:decoder)
choller: review+
Details | Diff | Splinter Review
Patch with adjusted wording in comment (1.07 KB, patch)
2011-12-14 09:57 PST, Christian Holler (:decoder)
choller: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2011-12-10 09:14:32 PST
I've been experimenting with address sanitizer and it triggered a startup crash in dom/base/nsDOMClassInfo.cpp, related to the "interface compacting" in the macro DOM_CLASS_MAPINFO_END.

The code looks as follows (fprintf added by me because gdb was being unfriendly):

    /* Compact the interface list */
    size_t count = ArrayLength(interface_list);
    fprintf(stderr, "Initial count is: %lu\n", count);
    /* count is the number of array entries, which is one greater than the */
    /* number of interfaces due to the terminating null */
    for (size_t i = 0; i < count - 1; ++i) {
      fprintf(stderr, "Current count is: %lu %lu\n", count, i);
      if (!interface_list[i]) {
        fprintf(stderr, "Performing memmove\n");
        memmove(&interface_list[i], &interface_list[i+1],
                sizeof(nsIID*) * (count - i));
        /* Make sure to examine the new pointer we ended up with at this */
        /* slot, since it may be null too */
        --i;
        --count;
      }
    }

With this code I get:
Initial count is: 8
Current count is: 8 0
Current count is: 8 1
Current count is: 8 2
Current count is: 8 3
Current count is: 8 4
Current count is: 8 5
Current count is: 8 6
Performing memmove
==5011== ERROR: AddressSanitizer global-buffer-overflow on address 0x7ff38a77d8c7 at pc 0x7ff385dc5000 bp 0x7fff9b0d8e50 sp 0x7fff9b0d8e48
READ of size 1 at 0x7ff38a77d8c7 thread T0


As far as I can see, with count=8 and i=6, the memmove will move 2 entries (interface_list[i+1] and interface_list[i+2]) where the second is off-by-one. I fixed this by using sizeof(nsIID*) * (count - i - 1) instead.

Can somebody with more knowledge of this code area confirm and fix this?
Comment 1 Christian Holler (:decoder) 2011-12-12 17:51:28 PST
Created attachment 581126 [details] [diff] [review]
Patch

Fix as proposed in comment 0.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-12-13 21:41:17 PST
Comment on attachment 581126 [details] [diff] [review]
Patch

Yeah, good catch.  And really sorry for the lag here!

I'd prefer we write that as |count - (i + 1)| with a comment about how the number of entries we have examined so far is i+1.  r=me with that.
Comment 3 Christian Holler (:decoder) 2011-12-14 04:29:57 PST
Created attachment 581589 [details] [diff] [review]
Updated patch

Updated patch according to comments, carrying r+ from last review.
Comment 4 Christian Holler (:decoder) 2011-12-14 04:31:30 PST
Created attachment 581590 [details] [diff] [review]
Updated patch

This time it's really the updated patch ;) r+ from previous review.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-12-14 09:49:10 PST
Comment on attachment 581590 [details] [diff] [review]
Updated patch

Actually, instead of "(i+1)-th element" it would be better to say "element at index i+1".  That way there's no confusion about 1-based vs 0-based counting....

Note that my proposed wording explicitly avoided the basing issue by talking abotu the number of elements we have examined.  I'd still prefer that wording.
Comment 6 Christian Holler (:decoder) 2011-12-14 09:57:51 PST
Created attachment 581693 [details] [diff] [review]
Patch with adjusted wording in comment

Updated once more for the comment, r+ from last patch :)
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-12-14 10:04:24 PST
'i+1' -> 'i + 1' 4 times, please.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-12-14 22:27:41 PST
For future reference, it would be really nice to have a User line and checkin comment in the diff!

https://hg.mozilla.org/mozilla-central/rev/a3f62505cd16

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