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

RESOLVED FIXED in mozilla11



6 years ago
6 years ago


(Reporter: decoder, Assigned: decoder)


Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [asan])


(1 attachment, 3 obsolete attachments)



6 years ago
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?

Comment 1

6 years ago
Created attachment 581126 [details] [diff] [review]

Fix as proposed in comment 0.
Assignee: nobody → choller
Attachment #581126 - Flags: review?(bzbarsky)


6 years ago
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+

Comment 3

6 years ago
Created attachment 581589 [details] [diff] [review]
Updated patch

Updated patch according to comments, carrying r+ from last review.
Attachment #581126 - Attachment is obsolete: true
Attachment #581589 - Flags: review+

Comment 4

6 years ago
Created attachment 581590 [details] [diff] [review]
Updated patch

This time it's really the updated patch ;) r+ from previous review.
Attachment #581589 - Attachment is obsolete: true
Attachment #581590 - Flags: review+


6 years ago
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.

Comment 6

6 years ago
Created attachment 581693 [details] [diff] [review]
Patch with adjusted wording in comment

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!
Last Resolved: 6 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.