1486706 - Audit callers of nsTSubstring::SetCapacity() for subsequent writing past the logical length of the string via BeginWriting() in c-c

Status: RESOLVED FIXED

Description

[Henri Sivonen (:hsivonen) (not reading bugmail until 2019-08-05)]

This is the c-c version of bug 1472113.

See bug 1484668 for the details and to-be-landed documentation in the patch. The reason why the documentation hasn't landed is that it refers to an API that's pending review in bug 1482828.

Please audit c-c to see if this pattern occurs:
 1) Call SetCapacity() on a string
 2) Use BeginWriting() to write past mLength of the string but staying within the capacity set in the first step.
 3) Call SetLength() with the number of code units actually written.

If that pattern occurs, please change things so that it doesn't occur either by changing the call to SetCapacity() to SetLength() (if applicable given other calls to the string), by replacing the write via BeginWriting() with a call to Append() or by using the BulkWrite() API to be added in bug 1482828.

Comment 1

[Henri Sivonen (:hsivonen) (not reading bugmail until 2019-08-05)]

Please change these two SetCapacity calls to SetLength instead:
https://searchfox.org/comm-central/source/mailnews/base/util/nsMsgI18N.cpp#102
https://searchfox.org/comm-central/source/mailnews/base/util/nsMsgI18N.cpp#139

That's it!

Comment 2

[Jorg K (GMT+2)]

Attachment: 1486706-setLength.patch

Thanks for the heads-up, Henri. It's not quite clear to me which bug we're porting here. Can you please give me a number. Would current usage break at some stage?

Comment 3

[Jorg K (GMT+2)]

Attachment: 1486706-setLength.patch (v1b)

Oops, spurious spaces removed.

Comment 4

[Henri Sivonen (:hsivonen) (not reading bugmail until 2019-08-05)]

(In reply to Jorg K (GMT+2) from comment #2)
> Thanks for the heads-up, Henri. It's not quite clear to me which bug we're
> porting here. Can you please give me a number. Would current usage break at
> some stage?

This is in preparation for bug 1487341 and bug 1486711. And yes, those would break current usage.

Comment 5

[Henri Sivonen (:hsivonen) (not reading bugmail until 2019-08-05)]

Comment on attachment 9006050 [details] [diff] [review]
1486706-setLength.patch (v1b)

Review of attachment 9006050 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5f30a26c656a
Prepare for bug 1487341: replace use of SetCapacity() with SetLength (). r=hsivonen