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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Cameron McCormack (:heycam)]

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.

Comment 2

[Chris Peterson [:cpeterson]]

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.

Comment 3

[Mike Hommey [:glandium]]

Note that, as a matter of fact, this *is* happening on a PGO build with gcc5.

Comment 4

[Mike Hommey [:glandium]]

(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.

Comment 5

[Mike Hommey [:glandium]]

(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.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Disable -Warray-bounds as a fatal warning

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 8592100 [details] [diff] [review]
Disable -Warray-bounds as a fatal warning

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

LGTM.

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b86dac3ed1b

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b86dac3ed1b

Comment 11

[Trevor Saunders (:tbsaunde)]

(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.