1472113 - [meta] Audit callers of SetCapacity for writing past mLength

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Henri Sivonen (:hsivonen)]

Currently, we have callers that assume that it's valid to do this:
 1) Call SetCapacity()
 2) Obtain a raw pointer to the start of the string and write past mLength of the string.
 3) Call SetLength() with a new length assuming that even a reallocation happens, the data written past mLength is preserved.

This is bad, because it causes excessive memcpy in the case of reasonable callers.

We should audit callers of SetCapacity() to make sure that callers that wish to write stuff to the raw buffer use SetLength() instead. Then we should make SetCapacity() preserve buffer data only up to mLength.

It doesn't make sense to start this until bug 1402247 has landed.

Comment 1

[Henri Sivonen (:hsivonen)]

Base64Decode is one of the bad callers.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Henri Sivonen (:hsivonen) from comment #0)
> Currently, we have callers that assume that it's valid to do this:
>  1) Call SetCapacity()
>  2) Obtain a raw pointer to the start of the string and write past mLength
> of the string.
>  3) Call SetLength() with a new length assuming that even a reallocation
> happens, the data written past mLength is preserved.

Is it really common for callers to pass different lengths to SetCapacity/SetLength, or even a *larger* SetLength than SetCapacity?

> This is bad, because it causes excessive memcpy in the case of reasonable
> callers.

I agree this is a gnarly pattern, but where does the excessive memcpy'ing come from?

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> (In reply to Henri Sivonen (:hsivonen) from comment #0)
> > Currently, we have callers that assume that it's valid to do this:
> >  1) Call SetCapacity()
> >  2) Obtain a raw pointer to the start of the string and write past mLength
> > of the string.
> >  3) Call SetLength() with a new length assuming that even a reallocation
> > happens, the data written past mLength is preserved.
> 
> Is it really common for callers to pass different lengths to
> SetCapacity/SetLength, or even a *larger* SetLength than SetCapacity?

In addition to the Base64 code, I found another offender:
https://searchfox.org/mozilla-central/source/gfx/layers/opengl/GLBlitTextureImageHelper.cpp#243

So far not common.

> > This is bad, because it causes excessive memcpy in the case of reasonable
> > callers.
> 
> I agree this is a gnarly pattern, but where does the excessive memcpy'ing
> come from?

Having to memcpy not only the number of code units indicated by the old mLength but the whole buffer--even the part that's logically supposed to be unused.

Comment 4

[Henri Sivonen (:hsivonen)]

More:
https://searchfox.org/mozilla-central/source/gfx/layers/opengl/GLBlitTextureImageHelper.cpp#267
https://searchfox.org/mozilla-central/source/gfx/layers/opengl/OGLShaderProgram.cpp#949
https://searchfox.org/mozilla-central/source/netwerk/base/Dashboard.cpp#889
https://searchfox.org/mozilla-central/source/netwerk/base/nsNetAddr.cpp#48
https://searchfox.org/mozilla-central/source/netwerk/base/nsNetAddr.cpp#53
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2564
https://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/Entries.h#94
https://searchfox.org/mozilla-central/source/xpcom/io/nsStorageStream.cpp#588

Bogus in different way:
https://searchfox.org/mozilla-central/source/netwerk/base/nsSocketTransport2.cpp#1347

Bogus in yet another way:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/Http2Compression.cpp#1095

Useless:
https://searchfox.org/mozilla-central/source/services/crypto/component/IdentityCryptoService.cpp#37

I think I've now audited all the calls to SetCapacity() in the codebase.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> Bogus in yet another way:
> https://searchfox.org/mozilla-central/source/netwerk/protocol/http/
> Http2Compression.cpp#1095

The use of BeginWriting() in this file is at the very least weird, though perhaps not wrong.

Comment 6

[Henri Sivonen (:hsivonen)]

XHR's responseText building has this issue, too.

Comment 7

[Henri Sivonen (:hsivonen)]

Actual patches in the dependencies. Marking this one as meta.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Henri Sivonen (:hsivonen) from comment #0)
> Currently, we have callers that assume that it's valid to do this:
>  1) Call SetCapacity()
>  2) Obtain a raw pointer to the start of the string and write past mLength
> of the string.
>  3) Call SetLength() with a new length assuming that even a reallocation
> happens, the data written past mLength is preserved.

Would poisoning the truncated bytes of the buffer catch these bad callers? Trying to read beyond mLength would then return garbage data.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Chris Peterson [:cpeterson] from comment #8)
> (In reply to Henri Sivonen (:hsivonen) from comment #0)
> > Currently, we have callers that assume that it's valid to do this:
> >  1) Call SetCapacity()
> >  2) Obtain a raw pointer to the start of the string and write past mLength
> > of the string.
> >  3) Call SetLength() with a new length assuming that even a reallocation
> > happens, the data written past mLength is preserved.
> 
> Would poisoning the truncated bytes of the buffer catch these bad callers?
> Trying to read beyond mLength would then return garbage data.

Yeah, that would make sense to do here once the dependencies have been fixed.

Comment 10

[Henri Sivonen (:hsivonen)]

All the dependencies have landed.