Closed Bug 334571 Opened 14 years ago Closed 10 years ago

Coverity 702, NS_SIDES array limit check

Categories

(Core :: CSS Parsing and Computation, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jonsmirl, Assigned: timeless)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(4 files, 6 obsolete files)

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.
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)
I don't care about the naming convention.
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
enum Sides { NS_SIDE_TOP, ... };
is just fine.

Things doing looks should probably be using the NS_FOR_CSS_SIDES macro anyway.
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.
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.
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?
Do we have other global enums to use as precedent?

operator++ should return a value.
Attached patch Updated version (obsolete) — Splinter Review
Fixes operator++ to return a value. 
All loops now use the macro.
enum name changed to CSSsides instead of Sides.
diff -u
Attachment #220268 - Attachment is obsolete: true
Attachment #220274 - Flags: review?(dbaron)
(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.
And the operator++ should assert that the values both before and after are valid side values.
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.
Attachment #220274 - Flags: review?(dbaron) → review-
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.
Attachment #218925 - Attachment is obsolete: true
Attachment #220274 - Attachment is obsolete: true
Attachment #224803 - Flags: review?(dbaron)
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 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.)
Attachment #224803 - Flags: superreview?(roc)
Attachment #224803 - Flags: review?(vladimir)
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.
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. 
Jon, if you attach an up-to-date version of the patch I'll try to review it more quickly this time.
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.
(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.
Attachment #224803 - Flags: superreview?(roc)
Attachment #224803 - Flags: review?(vladimir)
Attachment #224803 - Flags: review?(dbaron)
Product: Core → Core Graveyard
Attached patch merged to trunk (obsolete) — Splinter Review
Assignee: general → timeless
Attachment #224803 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #431329 - Flags: review?(dbaron)
Component: GFX → Style System (CSS)
Product: Core Graveyard → Core
QA Contact: ian → style-system
Attachment #431329 - Flags: review?(dbaron) → review?(roc)
Attachment #431329 - Flags: review?(roc) → review?(zweinberg)
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.
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.
mozilla::css::Side works for me.
i filed bug 551422 about comment 23

zwol: yeah, the corners thing was odd. the define should be easy.
Attached patch namespace mozilla::css (obsolete) — Splinter Review
Attachment #431329 - Attachment is obsolete: true
Attachment #431718 - Flags: review?(zweinberg)
Attachment #431329 - Flags: review?(zweinberg)
Attachment #431718 - Flags: review?(zweinberg) → review-
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.
(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.
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....
i'd like to land this early, although it might get stuck in my queue and land with the actual work.
Attachment #432378 - Flags: review?(zweinberg)
Attached patch Corner stuffSplinter Review
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.
Attachment #431718 - Attachment is obsolete: true
Attachment #432379 - Flags: review?(zweinberg)
Comment on attachment 432378 [details] [diff] [review]
whitespace cleanup (prereq)

rs=zwol
Attachment #432378 - Flags: review?(zweinberg) → review+
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.
Attachment #432379 - Flags: review?(zweinberg) → review+
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...
Actually, the second one should be
    NS_PRECONDITION(side >= NS_SIDE_TOP, "Out of range side");
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 :(
Attachment #432379 - Flags: review-
Attached patch fix assertionsSplinter Review
This fixes the assertions.  (Applies on top of your earlier two patches.)
Attachment #439425 - Flags: review?(timeless)
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)
Attachment #439426 - Flags: review?(timeless)
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
Attachment #439425 - Flags: review?(timeless) → review+
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
Attachment #439426 - Flags: review?(timeless) → review+
I think you should add a note about it.
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.
You need to log in before you can comment on or make changes to this bug.