Closed
Bug 1299102
Opened 8 years ago
Closed 8 years ago
[Static Analysis][Dereference after null check] In function nsCaseTransformTextRunFactory::TransformString
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: andi, Assigned: andi)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID 1372296)
Attachments
(1 file)
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 hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
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.
Updated•8 years ago
|
Attachment #8786296 -
Flags: review?(dbaron) → review?(jfkthame)
Comment 3•8 years ago
|
||
mozreview-review |
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.
Attachment #8786296 -
Flags: review?(jfkthame) → review-
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
mozreview-review |
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.
Attachment #8786296 -
Flags: review?(jfkthame) → review+
Comment hidden (mozreview-request) |
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4204b1030f2
swap NS_PRECONDITION with MOZ_ASSERT in TransformString. r=jfkthame
Comment 8•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•