1299102 - [Static Analysis][Dereference after null check] In function nsCaseTransformTextRunFactory::TransformString

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andi-Bogdan Postelnicu [:andi]]

The Static Analysis tool Coverity detected that pointer |aTextRun| is dereferenced after is being null checked witch implies a possible null pointer dereference.

Null check:

>>    if (aTextRun) {
>>      charStyle = aTextRun->mStyles[aOffsetInTextRun];
>>      style = aAllUppercase ? NS_STYLE_TEXT_TRANSFORM_UPPERCASE :
>>        charStyle->mTextTransform;
>>      forceNonFullWidth = charStyle->mForceNonFullWidth;

Dereference:

>>      if (auxiliaryOutputArrays) {
>>        aStyleArray->AppendElement(charStyle);
>>        aCanBreakBeforeArray->AppendElement(
>>          inhibitBreakBefore ? gfxShapedText::CompressedGlyph::FLAG_BREAK_TYPE_NONE
>>                             : aTextRun->CanBreakBefore(aOffsetInTextRun));
>>      }

Comment 1

[Andi-Bogdan Postelnicu [:andi]]

Attachment: Bug 1299102 - swap NS_PRECONDITION with MOZ_ASSERT in TransformString.

Review commit: https://reviewboard.mozilla.org/r/75272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75272/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8786296 [details]
Bug 1299102 - swap NS_PRECONDITION with MOZ_ASSERT in TransformString.

https://reviewboard.mozilla.org/r/75272/#review73162

Hmmm... actually it isn't needed, given there is a precondition at the beginning of this function which asserts "!auxiliaryOutputArrays || aTextRun", so presumably this wouldn't happen. Probably we can just replace the NS_PRECONDITION to MOZ_ASSERT to be sure.

Comment 3

[Jonathan Kew (:jfkthame)]

Comment on attachment 8786296 [details]
Bug 1299102 - swap NS_PRECONDITION with MOZ_ASSERT in TransformString.

https://reviewboard.mozilla.org/r/75272/#review73336

As Xidorn noted, the extra test is not needed here; if `auxiliaryOutputArrays` is true, `aTextRun` will always be non-null, as per the precondition at the top of the method. Any violation of this precondition indicates a fundamental coding error in the caller.

If it makes coverity any happier, I will r+ a patch that changes the NS_PRECONDITION there into a MOZ_ASSERT.

Comment 4

[Andi-Bogdan Postelnicu [:andi]]

Comment on attachment 8786296 [details]
Bug 1299102 - swap NS_PRECONDITION with MOZ_ASSERT in TransformString.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75272/diff/1-2/

Comment 5

[Jonathan Kew (:jfkthame)]

Comment on attachment 8786296 [details]
Bug 1299102 - swap NS_PRECONDITION with MOZ_ASSERT in TransformString.

https://reviewboard.mozilla.org/r/75272/#review73526

::: layout/generic/nsTextRunTransformations.cpp:289
(Diff revision 2)
> -  NS_PRECONDITION(!auxiliaryOutputArrays || aTextRun,
> +  MOZ_ASSERT(!auxiliaryOutputArrays || aTextRun,
>                    "text run must be provided to use aux output arrays");

Please also adjust the indent of the wrapped line, to maintain the alignment.

Comment 6

[Andi-Bogdan Postelnicu [:andi]]

Comment on attachment 8786296 [details]
Bug 1299102 - swap NS_PRECONDITION with MOZ_ASSERT in TransformString.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75272/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4204b1030f2
swap NS_PRECONDITION with MOZ_ASSERT in TransformString. r=jfkthame

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a4204b1030f2