[gcc 5.0] layout/style/nsCSSParser.cpp:14517:30: error: array subscript is above array bounds [-Werror=array-bounds]

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks 1 bug)

unspecified
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

When building with gcc 5.0 and --enable-warning-as-errors:

23:50:53     INFO -  In file included from /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/layout/style/Unified_cpp_layout_style1.cpp:56:0:
23:50:53     INFO -  /builds/slave/try-lx-00000000000000000000000/build/src/layout/style/nsCSSParser.cpp: In member function '{anonymous}::CSSParserImpl::ParseAnimationOrTransitionShorthandResult {anonymous}::CSSParserImpl::ParseAnimationOrTransitionShorthand(const nsCSSProperty*, const nsCSSValue*, nsCSSValue*, size_t)':
23:50:53     INFO -  /builds/slave/try-lx-00000000000000000000000/build/src/layout/style/nsCSSParser.cpp:14517:30: error: array subscript is above array bounds [-Werror=array-bounds]
23:50:53     INFO -           if (!parsedProperty[i]) {
23:50:53     INFO -                                ^
(removed irrelevant -Wno-error warnings)
23:50:53     INFO -  cc1plus: all warnings being treated as errors

It's actually right. While there's a MOZ_ASSERT on aNumProperties <= maxNumProperties, that expands to nothing on opt builds, so the compiler doesn't know it's supposed to be safe.

Now the question is whether we want to keep this particular warning as error, and fix this error somehow, or make it -Wno-error.
The code is safe -- the two callers of ParseAnimationOrTransitionShorthand only ever pass in aNumProperties == 4 and aNumProperties == 8, so we will never read past the end of the array -- but I'm not sure what the best way to let the compiler know this is without a runtime check.  So I would say disable the warning here if that is easy.
I have a local patch to disable -Warray-bounds due to a tricky false positive in Linux gcc PGO builds. I don't think I have ever seen a legitimate -Warray-bounds warning before. :)

Would declaring ParseAnimationOrTransitionShorthand() as a private member function of class CSSParserImpl suppress the warning? gcc might have an easier time seeing that no other callers or derived classes will call ParseAnimationOrTransitionShorthand() with a bad array index.
Blocks: buildwarning
Note that, as a matter of fact, this *is* happening on a PGO build with gcc5.
(In reply to Chris Peterson [:cpeterson] from comment #2)
> Would declaring ParseAnimationOrTransitionShorthand() as a private member
> function of class CSSParserImpl suppress the warning? gcc might have an
> easier time seeing that no other callers or derived classes will call
> ParseAnimationOrTransitionShorthand() with a bad array index.

I pushed a try changing a "protected" to a "private" in the CSSParserImpl declaration.
(In reply to Mike Hommey [:glandium] from comment #4)
> I pushed a try changing a "protected" to a "private" in the CSSParserImpl
> declaration.

And that didn't work.
Duplicate of this bug: 1154151
Assignee: nobody → mh+mozilla
Attachment #8592100 - Flags: review?(cpeterson)
Comment on attachment 8592100 [details] [diff] [review]
Disable -Warray-bounds as a fatal warning

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

LGTM.
Attachment #8592100 - Flags: review?(cpeterson) → review+
https://hg.mozilla.org/mozilla-central/rev/7b86dac3ed1b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(In reply to Chris Peterson [:cpeterson] from comment #2)
> I have a local patch to disable -Warray-bounds due to a tricky false
> positive in Linux gcc PGO builds. I don't think I have ever seen a
> legitimate -Warray-bounds warning before. :)

I think bug 966992 is a example, and arguably bug 854105.  However yeah, it has far too many false positives and I haven't looked at this case.
You need to log in before you can comment on or make changes to this bug.