Closed Bug 709483 Opened 9 years ago Closed 9 years ago

Off-by-one in dom/base/nsDOMClassInfo.cpp


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: decoder, Assigned: decoder)



(Whiteboard: [asan])


(1 file, 3 obsolete files)

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 */

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?
Attached patch Patch (obsolete) — Splinter Review
Fix as proposed in comment 0.
Assignee: nobody → choller
Attachment #581126 - Flags: review?(bzbarsky)
Blocks: 664901
Comment on attachment 581126 [details] [diff] [review]

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.
Attachment #581126 - Flags: review?(bzbarsky) → review+
Attached patch Updated patch (obsolete) — Splinter Review
Updated patch according to comments, carrying r+ from last review.
Attachment #581126 - Attachment is obsolete: true
Attachment #581589 - Flags: review+
Attached patch Updated patch (obsolete) — Splinter Review
This time it's really the updated patch ;) r+ from previous review.
Attachment #581589 - Attachment is obsolete: true
Attachment #581590 - Flags: review+
Keywords: checkin-needed
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.
Updated once more for the comment, r+ from last patch :)
Attachment #581590 - Attachment is obsolete: true
Attachment #581693 - Flags: review+
'i+1' -> 'i + 1' 4 times, please.
For future reference, it would be really nice to have a User line and checkin comment in the diff!
Closed: 9 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.