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)

defect
Not set
normal

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 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.
Attachment #8786296 - Flags: review?(dbaron) → review?(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.
Attachment #8786296 - Flags: review?(jfkthame) → 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+
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a4204b1030f2 swap NS_PRECONDITION with MOZ_ASSERT in TransformString. r=jfkthame
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.

Attachment

General

Creator:
Created:
Updated:
Size: