334571 - Coverity 702, NS_SIDES array limit check

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jon Smirl]

Sides are #define'd currently

#define NS_SIDE_TOP     0
#define NS_SIDE_RIGHT   1
#define NS_SIDE_BOTTOM  2
#define NS_SIDE_LEFT    3

I believe if these were an enum Coverity could correctly catch array out of bounds. Currently it is flagging all references into sides[4] arrays.

enum NS_SIDES {NS_SIDE_TOP, NS_SIDE_RIGHT, NS_SIDE_BOTTOM, NS_SIDE_LEFT};

If attached header changes are ok, I'll make the needed adjustments in the code to propagate the NS_SIDES enum.

Comment 1

[Jon Smirl]

Attachment: Proposed change from #define to enum

Comment 2

[Bernd]

Doesn't the patch break the naming convention http://www.mozilla.org/newlayout/doc/codingconventions.html ( this is old and I am not sure that it still applies)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't care about the naming convention.

Comment 4

[Jon Smirl]

If the names are changed to the enum convention there are 479 places where they would need to be patched up. Plus any external apps. Since these are old defines I would leave the names alone.

I did notice multiple places where 1,2,3,4 were being used instead of one the side defines. 

Something like this could be done and references to NS_SIDE_xx could be removed over time.
enum Sides {eSideTop, eSideRight, eSideBottom, eSideLeft);
#define NS_SIDE_TOP eSideTop
#define NS_SIDE_RIGHT eSideRight

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

enum Sides { NS_SIDE_TOP, ... };
is just fine.

Things doing looks should probably be using the NS_FOR_CSS_SIDES macro anyway.

Comment 6

[Jon Smirl]

Attachment: Complete change for making sides into enum

This is the full patch needed to convert NS_SIDES_XXX to use an enum instead of #defines. Making this change will allow Coverity to range check arrary references into arrays of sides instead of flagging them all as errors. The patch makes no functional change to the code, it only changes variable types and declarations.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sides might be too generic a name.

Is ++ on an enum portable?  IIRC, v = v + 1 is not.

Might it be better to convert the two or three occurrences of for loops over sides that don't use the NS_FOR_CSS_SIDES macro to use the macro?

Please attach unified diffs.  Diffs without any context are quite dangerous, and unified diffs are more readable.

I haven't looked very closely at the patch.

Comment 8

[Jon Smirl]

What would you like for a name instead of sides?

There is an inline doing the ++ for the enum.
static inline void operator++(Sides& side, int) {
    side = Sides(side+1);
}

The files using the loops doing include nsStyleConsts.h. You want me to add the includes and switch to the macro?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we have other global enums to use as precedent?

operator++ should return a value.

Comment 10

[Jon Smirl]

Attachment: Updated version

Fixes operator++ to return a value. 
All loops now use the macro.
enum name changed to CSSsides instead of Sides.
diff -u

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> All loops now use the macro.

Not the one in nsFrame.h.

> enum name changed to CSSsides instead of Sides.

Still a bad name, for at least the following reasons:
 * the value is a side, not a set of sides, so it shouldn't have a final s
 * it doesn't follow any local naming conventions.  Possibilities are nsCSSSide, gfxSide, etc.  If you're putting it in gfxCore.h you should probably check with pav and/or roc; I prefer gfxSide.

Please don't remove the assertions from code that depends on the side constants.  ****If**** we don't have a static assertion macro already, copy JS_STATIC_ASSERT from jsutil.h to an NS_STATIC_ASSERTION macro in nsDebug.h and then use that.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And the operator++ should assert that the values both before and after are valid side values.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 220274 [details] [diff] [review]
Updated version

I think the patch would be acceptable to me with the changes I mention above and below, but it needs to be run by some other people as well.

Also, this one:
-  for (cnt = 0; cnt < 4; cnt++) {
-    PRUint8 side = sideOrder[cnt];
+  NS_FOR_CSS_SIDES(cnt) {
+    CSSsides side = sideOrder[cnt];
should *not* use the macro.  It's not iterating over the sides, it's iterating over an array whose values are sides.

Also, the macro should probably have a name equivalent to the name of the type; if it's gfxSide then the macro should be renamed to NS_FOR_GFX_SIDES.

Comment 14

[Jon Smirl]

Attachment: New version addressing issues raised

There is no way to get the sides enum out of gfxcore.h without reworking a lot of header files. I tried but it was getting too complex.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One other note to self:  NS_STATIC_ASSERT as written here doesn't work well in header files because of the names it uses -- names can collide.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 224803 [details] [diff] [review]
New version addressing issues raised

Vlad and roc, do you think we should do this?  I don't want to put more energy into reviewing this unless we actually want it.

(It's also probably bitrotted a good bit.)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's worth doing, although the name should be changed from gfxSide to nsSide since I think we want gfx* types to be restricted to Thebes.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm not a big fan of NS_FOR_GFX_SIDES(); I'd rather we ditch that and just define a NS_SIDE_MAX as prat of the enum. 

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Jon, if you attach an up-to-date version of the patch I'll try to review it more quickly this time.

Comment 20

[Jon Smirl]

I haven't been working on Mozilla for the last year. This is multiple times now that I've put together significant patches that languished in Bugzilla for one or more years. Also, failure to switch to a distributed version control system like git makes it too much trouble to maintain out of tree patches over long periods of time. Currently I'm running a company doing a Linux based embedded audio device. It may be quite a while until I work on Mozilla again.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #18)
> I'm not a big fan of NS_FOR_GFX_SIDES(); I'd rather we ditch that and just
> define a NS_SIDE_MAX as prat of the enum. 

For what it's worth, that defeats the point of the patch, since the compiler and static analysis tools won't then know that we don't need to check NS_SIDE_MAX as well.

Comment 22

[timeless]

Attachment: merged to trunk

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I did notice that perhaps #include "nspr.h" should be #include "prlog.h"; we don't need to drag in all NSPR headers to get PR_STATIC_ASSERT.

And if we're including prlog.h in a major header file we probably should re-audit the users of FORCE_PR_LOG to make sure they're all defining it *before* including any headers.

Comment 24

[Zack Weinberg (:zwol)]

So before I go through this line-by-line, it seems like we have an unresolved conflict between dbaron and roc over the name of the enum.  dbaron favors gfxSide, roc favors nsSide.  I don't much care, although I'd be tempted to invent something in namespace mozilla :-)

I noticed that in at least one place you're using NS_FOR_GFX_SIDES for an iteration over *corners*, which is asking for confusion later, even though it's the same loop.  Please add NS_FOR_GFX_CORNERS and a corner constant enumeration.  You don't have to go through all the *uses* of the corner constants in this bug, though.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

mozilla::css::Side works for me.

Comment 26

[timeless]

i filed bug 551422 about comment 23

zwol: yeah, the corners thing was odd. the define should be easy.

Comment 27

[timeless]

Attachment: namespace mozilla::css

Comment 28

[Zack Weinberg (:zwol)]

Comment on attachment 431718 [details] [diff] [review]
namespace mozilla::css

>+namespace mozilla {
>+  namespace css {
>+    enum Side {top, right, bottom, left};
>+  };
>+};
>+#define NS_SIDE_TOP     mozilla::css::top
>+#define NS_SIDE_RIGHT   mozilla::css::right
>+#define NS_SIDE_BOTTOM  mozilla::css::bottom
>+#define NS_SIDE_LEFT    mozilla::css::left

This seems a bit too namespace-pollute-y -- there are places
(e.g. nsCSSParser.cpp) where "using namespace mozilla::css" may appear
in the future (ugly, yes, I know), and that would preclude the use of
the bare words "top", "left", "bottom", "right".

We don't AFAIK have a convention for enumeration constants in
namespaced code.  Since for now actual uses of these constants
continue to be hidden behind macros, I would be fine with either

  enum Side { side_top, side_right, side_bottom, side_left };

or

  enum Side { eSideTop, eSideRight, eSideBottom, eSideLeft };

and we can argue about what the *right* naming convention is if and
when we get around to removing the macros.  Please don't mix
underscores and camelcase, though.

>--- a/gfx/public/nsMargin.h
>+++ b/gfx/public/nsMargin.h
>@@ -41,6 +41,10 @@
> #include "nsCoord.h"
> #include "nsPoint.h"
> #include "gfxCore.h"
>+#include "prlog.h"
>+
>+// the mozilla::css::Side sequence must match the nsMargin nscoord sequence
>+PR_STATIC_ASSERT((NS_SIDE_TOP == 0) && (NS_SIDE_RIGHT == 1) && (NS_SIDE_BOTTOM == 2) && (NS_SIDE_LEFT == 3));

Don't include prlog.h in headers.  Move the assertion to a .cpp file;
I suggest nsRect.cpp.

>-struct THEBES_API gfxCorner {
>-    typedef int Corner;
>-    enum {
>+namespace mozilla {
>+namespace css {
>+enum Corner {
>         // this order is important!
>-        TOP_LEFT = 0,
>-        TOP_RIGHT = 1,
>-        BOTTOM_RIGHT = 2,
>-        BOTTOM_LEFT = 3,
>-        NUM_CORNERS = 4
>-    };
>+        topLeft = 0,
>+        topRight = 1,
>+        bottomRight = 2,
>+        bottomLeft = 3,
>+        numCorners = 4
> };
>+};
>+};
>+#define NS_TOP_LEFT mozilla::css::topLeft
>+#define NS_TOP_RIGHT mozilla::css::topRight
>+#define NS_BOTTOM_RIGHT mozilla::css::bottomRight
>+#define NS_BOTTOM_LEFT mozilla::css::bottomLeft
>+#define NS_NUM_CORNERS mozilla::css::numCorners

The same name-pollution criticism applies here.  We could get away
with unprefixed constant names in the old version because the
enumeration was nested inside the gfxCorner class, so the constants
actually got namespaced.

Also, the macros should be NS_CORNER_TOP_LEFT etc, for consistency
with NS_SIDE_* (NS_NUM_CORNERS is fine as is)

>-    gfxPoint Corner(gfxCorner::Corner corner) const {
>+    gfxPoint Corner(mozilla::css::Side corner) const {
>         switch (corner) {
>-            case gfxCorner::TOP_LEFT: return TopLeft();
>-            case gfxCorner::TOP_RIGHT: return TopRight();
>-            case gfxCorner::BOTTOM_LEFT: return BottomLeft();
>-            case gfxCorner::BOTTOM_RIGHT: return BottomRight();
>+            case NS_SIDE_TOP: return TopLeft();
>+            case NS_SIDE_RIGHT: return TopRight();
>+            case NS_SIDE_BOTTOM: return BottomLeft();
>+            case NS_SIDE_LEFT: return BottomRight();

Why on earth does this take a *side* constant?!  If this is actually
what users of this function expect, it should be renamed CCWCorner and
have a documentation comment expanding that acronym ("returns the corner
immediately counterclockwise to the side passed in" would be enough).

>+  NS_FOR_CSS_SIDES(i) {

If you've gone back to NS_FOR_CSS_SIDES() then the corner macro should
be NS_FOR_CSS_CORNERS() for consistency.  Also, do we put a space before
the open parenthesis on control-flow macros like this one?

>+#define NEXT_SIDE(_s) mozilla::css::Side(((_s) + 1) & 3)
>+#define PREV_SIDE(_s) mozilla::css::Side(((_s) + 3) & 3)

Did you verify that this produces no warnings about doing arithmetic
on enumeration constants with any compiler of interest?

>-  if (!IsZeroSize(mBorderRadii[aSide]))
>+  if (!IsZeroSize(mBorderRadii[mozilla::css::Corner(aSide)]))
...
>-  if (!IsZeroSize(mBorderRadii[NEXT_SIDE(aSide)]))
>+  if (!IsZeroSize(mBorderRadii[mozilla::css::Corner(NEXT_SIDE(aSide))]))

Maybe we want free functions CWCorner() and CCWCorner() that do this
conversion?  Also, I think a lot of this would be made more readable
by judicious use of

  namespace css = mozilla::css;

and then omitting the mozilla:: (in .cpp files only, of course).  But
that can be a separate patch, or you can not do it at all if you don't
want to.

>-#define C_TL (gfxCorner::TOP_LEFT)
>-#define C_TR (gfxCorner::TOP_RIGHT)
>-#define C_BR (gfxCorner::BOTTOM_RIGHT)
>-#define C_BL (gfxCorner::BOTTOM_LEFT)
>+#define C_TL (NS_TOP_LEFT)
>+#define C_TR (NS_TOP_RIGHT)
>+#define C_BR (NS_BOTTOM_RIGHT)
>+#define C_BL (NS_BOTTOM_LEFT)

Parentheses are unnecessary.

>-#define NS_SIDE_TOP     0
>-#define NS_SIDE_RIGHT   1
>-#define NS_SIDE_BOTTOM  2
>-#define NS_SIDE_LEFT    3
>+#define NS_SIDE_TOP     mozilla::css::top
>+#define NS_SIDE_RIGHT   mozilla::css::right
>+#define NS_SIDE_BOTTOM  mozilla::css::bottom
>+#define NS_SIDE_LEFT    mozilla::css::left

These macros should not be defined in two places.

>-  // aSide is always one of NS_SIDE_* defined in nsStyleConsts.h
>+  // aSide is always one of mozilla::css::* defined in nsStyleConsts.h

The mozilla::css::* you're talking about are not defined in
nsStyleConsts.h, but the comment itself is now unnecessary
because the new argument type expresses it, so just delete it.

>-  $
>+$

(dollar signs mine) Yay getting rid of trailing whitespace...

>@@ -2592,7 +2592,7 @@ void nsCellMap::Dump(PRBool aIsBorderCol
>     if (aIsBorderCollapse) {$
>       nscoord       size;$
>       BCBorderOwner owner;$
>-      PRUint8       side;$
>+      mozilla::css::Side side;$
>       PRBool        segStart;$
>       PRPackedBool  bevel;      $
>       for (PRInt32 i = 0; i <= 2; i++) {$

but if you're gonna, could you please do it consistently?

Otherwise looks fine, but I do want to see it after revisions.  Thanks
for slogging through this.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #28)
> >-  $
> >+$
> 
> (dollar signs mine) Yay getting rid of trailing whitespace...
> 
> >@@ -2592,7 +2592,7 @@ void nsCellMap::Dump(PRBool aIsBorderCol
> >     if (aIsBorderCollapse) {$
> >       nscoord       size;$
> >       BCBorderOwner owner;$
> >-      PRUint8       side;$
> >+      mozilla::css::Side side;$
> >       PRBool        segStart;$
> >       PRPackedBool  bevel;      $
> >       for (PRInt32 i = 0; i <= 2; i++) {$
> 
> but if you're gonna, could you please do it consistently?

I would avoid encouraging people to add whitespace cleanup to an existing patch.  It'll get hard to review very quickly.  If you want people to do whitespace cleanup, probably best to do it as separate patches.

Comment 30

[timeless]

Yeah, the whitespace changes were a mistake, and now i have to figure out how to undo them safely. I think what i'll do is post one patch which kills whitespace from all covered files and then another patch which has my changes. A rubber stamp to kill whitespace in all covered files would be appreciated.

> enum Side { eSideTop, eSideRight, eSideBottom, eSideLeft };

I'll choose this one, it's closer to Mozilla style.

> Also, the macros should be NS_CORNER_TOP_LEFT etc, for consistency
> with NS_SIDE_* (NS_NUM_CORNERS is fine as is)

yeah, i noticed that but was trying to get something up, note that at some point even *I* will lose interest, so it's better for me to post early and for you at some point to say "something is better than nothing" (it can take me years to resolve bugs, see a bug I pushed last week which is from 18xxxx iirc).

> Why on earth does this take a *side* constant?!  If this is actually
> what users of this function expect, it should be renamed CCWCorner and
> have a documentation comment expanding that acronym ("returns the corner
> immediately counterclockwise to the side passed in" would be enough).

In dealing with the code, it really seemed to prefer that, yes renaming it would make sense, but it was good to at least show what I experienced before I did random renames, now you feel my pain :).

> If you've gone back to NS_FOR_CSS_SIDES()

yeah, 'gfx' seemed to fall out of the changes (the namespace is css, and the gfx thing was contrived and churn for the sake of churn)

> then the corner macro should be NS_FOR_CSS_CORNERS() for consistency.

> Also, do we put a space before the open parenthesis on control-flow
> macros like this one?

It seemed that we generally did not.

> Did you verify that this produces no warnings about doing arithmetic
> on enumeration constants with any compiler of interest?

I'm only building on OS X 10.6.2 with probably my default compiler (4.2.1), I haven't started sending this to try (i spent last week sending 25 other changesets to try).

> Maybe we want free functions CWCorner() and CCWCorner() that do this
> conversion?

I think so, yes.

> Also, I think a lot of this would be made more readable by judicious use of
>  namespace css = mozilla::css;
> and then omitting the mozilla:: (in .cpp files only, of course). 
> But that can be a separate patch, or you can not do it at all if you don't
> want to.

I'm not very comfortable with namespaces, it can be done, but shouldn't be done for this initial stuff, I'll try to hide things behind macros in some cases.

> Parentheses are unnecessary.

Yeah, search+replace doesn't mean magical cleanup, will fix

> These macros should not be defined in two places.

Yeah, that actually bit me when compiling at one point (it struck me as odd even before that...).

I'll try to do my next iteration after dinner....

Comment 31

[timeless]

Attachment: whitespace cleanup (prereq)

i'd like to land this early, although it might get stuck in my queue and land with the actual work.

Comment 32

[timeless]

Attachment: Corner stuff

this should address most of the review comments. I'd like to avoid too many iterations of things. If possible, please allow me to do other things as further work (or to leave that work to someone else).

This has built on my mac, i haven't sent it to try for testing, i'm still open to perhaps 2 more rounds of reviews.

The Corner function was odd in that it wasn't ordered 0, 1, 2, 3. This version makes it use that order. I've renamed the Corner() form to AtCorner() mostly to show that I have indeed caught all instances that I know of. If there are some which are hidden behind ifdefs or something, renaming means I'll catch them when they're built. I'm not wed to the rename.

Comment 33

[Zack Weinberg (:zwol)]

Comment on attachment 432378 [details] [diff] [review]
whitespace cleanup (prereq)

rs=zwol

Comment 34

[Zack Weinberg (:zwol)]

Comment on attachment 432379 [details] [diff] [review]
Corner stuff

You still have the NS_SIDE_* constants being defined in both gfxCore.h and nsStyleConsts.h, but other than that I'm happy with this version.  Thanks for your patience.

Comment 35

[Mats Palmgren (inactive)]

The NS_PRECONDITIONs are backwards:
In gfxRect.h:
+    NS_PRECONDITION(corner < NS_CORNER_TOP_LEFT, "Out of range corner");
In nsStyleConsts.h:
+    NS_PRECONDITION(side < NS_SIDE_LEFT, "Out of range side");

There were a few reftest failures on TryServer (Windows only) in
layout/reftests/table-bordercollapse/*
The error was a missing pixel in a corner; I suspect it might be
a regression, so I re-submitted the patches to get a second run...

Comment 36

[Mats Palmgren (inactive)]

Actually, the second one should be
    NS_PRECONDITION(side >= NS_SIDE_TOP, "Out of range side");

Comment 37

[Mats Palmgren (inactive)]

Comment on attachment 432379 [details] [diff] [review]
Corner stuff

I'm afraid the table border reftest regression is real:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271293295.1271306567.4801.gz
I can also reproduce it locally, but only on Windows.

The problem is that 'enum' types are signed on this platform,
which causes problems when used as a bit field type.
A bit field with a value with MSB=1 will be sign-extended
when assigned to a wider type :(

Comment 38

[Mats Palmgren (inactive)]

Attachment: fix assertions

This fixes the assertions.  (Applies on top of your earlier two patches.)

Comment 39

[Mats Palmgren (inactive)]

Attachment: don't use enum bit fields

Leave the bit field type as the unsigned type it was before.
Cast the bit field value to the enum type where needed.
(Applies on top of the earlier patches)

Comment 40

[Mats Palmgren (inactive)]

Neither the Coding Style Guide [1] nor the C++ Portability Guide [2]
mentions this problem.  I think we should add a note about it.

[1] https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
[2] https://developer.mozilla.org/index.php?title=en/C%2b%2b_Portability_Guide

Comment 41

[timeless]

Comment on attachment 439426 [details] [diff] [review]
don't use enum bit fields

doh. fwiw, signed v. unsigned bit fields *is* something we've run into in mozilla before, i remember that, it was years ago, and i was a bystander. yeah, let's get this into the portability guide.

thanks a lot

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you should add a note about it.

Comment 43

[Mats Palmgren (inactive)]

I added a note to the Portability Guide:
<h3>Don't use an enum type for a bitfield</h3>
Enums are signed on some platforms and unsigned on others and therefore unsuitable for writing portable code.

Comment 44

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7aac8fafc4de
http://hg.mozilla.org/mozilla-central/rev/51954c20c9f7
http://hg.mozilla.org/mozilla-central/rev/d83245839ca6
http://hg.mozilla.org/mozilla-central/rev/22c58d0e27ee