1692123 - Potential Use-After-Free in `nsTArray_RelocateUsingMoveConstructor::RelocateNonOverlappingRegion`

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[unseddd]

How was it discovered: manual code review
Firefox version: mozilla-central changeset: 566812:569826c0fd47

Still working on a reproducer, since I found the code through static code review.

In mozilla-central/xpcom/ds/nsTArray.h:767-772 inside nsTArray_RelocateUsingMoveConstructor::RelocateNonOverlappingRegion:

    ElemType* destElem = static_cast<ElemType*>(aDest);
    ElemType* srcElem = static_cast<ElemType*>(aSrc);
    ElemType* destElemEnd = destElem + aCount;
#ifdef DEBUG
    ElemType* srcElemEnd = srcElem + aCount;
    MOZ_ASSERT(srcElemEnd <= destElem || srcElemEnd > destElemEnd);
#endif
    while (destElem != destElemEnd) {
      traits::Construct(destElem, std::move(*srcElem));
      traits::Destruct(srcElem);

The debug assertion MOZ_ASSERT(srcElemEnd <= destElem || srcElemEnd > destElemEnd); checks srcElemEnd > destElemEnd to ensure it is in a non-overlapping region, but the correct check would be srcElem > destElemEnd.

If srcElem <= destElemEnd and aCount >= 1, the traits::Destruct(srcElem) will result in a freed element in aDest. If aDest is used later to access the freed element(s), it will result in a use-after-free bug. It will also result in a double-free if aDest with a freed element is later freed.

I haven't traced anywhere in the codebase where this actually happens, so this may be non-exploitable. Will work on a unit-test for regression test and reproducibility.

Just wanted to let the team know as soon as I found it.

Comment 1

[Andrew McCreight (out until 2026) [:mccr8]]

Simon, could you take a look at this? Thanks.

Comment 2

[Simon Giesecke [:sg] [he/him]]

At first glance I think I agree that the assertion is wrong and should be fixed as suggested. However, this is only a debug assertion. If nothing else is wrong here, it's not a security issue and doesn't affect non-debug builds at all.

The caller(s) must ensure that the (corrected) precondition is not violated.

I will give it a closer look tomorrow.

Comment 3

[unseddd]

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

At first glance I think I agree that the assertion is wrong and should be fixed as suggested. However, this is only a debug assertion. If nothing else is wrong here, it's not a security issue and doesn't affect non-debug builds at all.

Simon, I agree, and wasn't sure whether to file it as a security bug. Wanted to be cautious, though.

The caller(s) must ensure that the (corrected) precondition is not violated.

I assumed this was the case, since RelocateNonOverlappingRegion seems to be an internal call.

I have a unit test that does the incorrect thing:

nsArray<int> a = {1, 2, 3, 4, 5};
nsTArray_RelocateUsingMoveConstructor::RelocateNonOverlappingRegion(&a[1], &a, 2, sizeof(int));

Wasn't sure if a unit test was the right place for that, or how to write a "this should fail" test.

I will give it a closer look tomorrow.

Thanks for your time reviewing this bug.

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to unseddd from comment #3)

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

At first glance I think I agree that the assertion is wrong and should be fixed as suggested. However, this is only a debug assertion. If nothing else is wrong here, it's not a security issue and doesn't affect non-debug builds at all.

Simon, I agree, and wasn't sure whether to file it as a security bug. Wanted to be cautious, though.

Sure, that's always a good idea in a case of doubt!

The caller(s) must ensure that the (corrected) precondition is not violated.

I assumed this was the case, since RelocateNonOverlappingRegion seems to be an internal call.

Yes, it is internal. It's only used from within nsTArray.h.

However, it's really somewhat intricate here. Despite the name saying "RelocateNonOverlappingRegion", the comment at https://searchfox.org/mozilla-central/rev/0379f315c75a2875d716b4f5e1a18bf27188f1e6/xpcom/ds/nsTArray.h#737-741 says it's also called for overlapping regions from within nsTArray_RelocateUsingMoveConstructor itself (so even more internally). So it can't be asserted here that the reasons don't overlap, but only that the preconditions for moving the range in a forward direction are met. This is surely confusing, and deserves some improvement. However, I think now that the code itself is actually right. We can't use std::move/std::move_backward here, since we actually "relocate" (move-construct at the destination location, destroy at the source location) rather than move-assign to the destination location, which isn't really reflected by the current description of the class. I'll provide a patch that improves this.

I have a unit test that does the incorrect thing:

nsArray<int> a = {1, 2, 3, 4, 5};
nsTArray_RelocateUsingMoveConstructor::RelocateNonOverlappingRegion(&a[1], &a, 2, sizeof(int));

Well, obviously, but there are lots of functions which have undefined behaviour in case their precondition is violated. That's not a defect. Theoretically, one could test that debug assertions capture all of these cases (in debug builds), but at least for now we don't do that (anywhere? not sure, but definitely not widely).

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1692123 - Refactor and improve documentation of nsTArray_RelocateUsingMoveConstructor. r=#xpcom-reviewers

Comment 6

[Simon Giesecke [:sg] [he/him]]

Can you check my thoughts, maybe together with the patch, and in case I haven't missed anything make this a non-security bug?

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I trust your judgment here, especially given that it's a debug-only assertion, so I don't think this is a security bug.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/093b814adf52
Refactor and improve documentation of nsTArray_RelocateUsingMoveConstructor. r=xpcom-reviewers,nika

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/093b814adf52

Comment 10

[Daniel Veditz [:dveditz] back January 5, 2026]

unseddd: Thank you for your in-depth code review. Unfortunately we don't believe this is exploitable in Firefox and does not qualify for a bug bounty