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

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

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)]

Attachment: Patch

Fix as proposed in comment 0.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: Updated patch

Updated patch according to comments, carrying r+ from last review.

Comment 4

[Christian Holler (:decoder)]

Attachment: Updated patch

This time it's really the updated patch ;) r+ from previous review.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: Patch with adjusted wording in comment

Updated once more for the comment, r+ from last patch :)

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

'i+1' -> 'i + 1' 4 times, please.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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