1299379 - ubsan: NS_FOR_CSS_SIDES creates invalid Side values

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Julian Seward [:jseward]]

Running an Ubsan'd build of the browser (-fsanitize=undefined) produces a 
large number of warnings like this:

layout/style/nsCSSParser.cpp:11111:60: runtime error: load of value 4, which is not a valid value for type 'Side'
layout/style/nsCSSParser.cpp:13432:60: runtime error: load of value 4, which is not a valid value for type 'Side'
layout/style/nsCSSParser.cpp:11268:58: runtime error: load of value 4, which is not a valid value for type 'Side'
layout/style/nsCSSParser.cpp:11291:58: runtime error: load of value 4, which is not a valid value for type 'Side'
layout/style/nsCSSParser.cpp:11089:62: runtime error: load of value 4, which is not a valid value for type 'Side'
layout/style/nsCSSParser.cpp:11073:60: runtime error: load of value 4, which is not a valid value for type 'Side'
layout/style/nsCSSParser.cpp:13762:60: runtime error: load of value 4, which is not a valid value for type 'Side'

This happens because of the NS_FOR_CSS_SIDES macro, which is used thusly:

  NS_FOR_CSS_SIDES (index) {
    AppendValue(aPropIDs[index], result.*(nsCSSRect::sides[index]));
  }

That expands to

  for (mozilla::css::Side index = NS_SIDE_TOP; index <= NS_SIDE_LEFT; index++) {
    AppendValue(aPropIDs[index], result.*(nsCSSRect::sides[index]));
  }

where

  static inline css::Side operator++(css::Side& side, int) {
      NS_PRECONDITION(side >= NS_SIDE_TOP &&
                      side <= NS_SIDE_LEFT, "Out of range side");
      side = css::Side(side + 1);
      return side;
  }

  // Side constants for use in various places.
  enum Side { eSideTop, eSideRight, eSideBottom, eSideLeft };

  #define NS_SIDE_TOP    mozilla::eSideTop     // 0
  #define NS_SIDE_RIGHT  mozilla::eSideRight
  #define NS_SIDE_BOTTOM mozilla::eSideBottom
  #define NS_SIDE_LEFT   mozilla::eSideLeft    // 3

So the problem is that, in the last iteration of the loop, with
index == Side(mozilla::eSideLeft), we construct Side(4), and then
exit the loop because the test Side(4) <= Side(3) fails.  But the
problem is that 4 isn't a valid value for "enum Side" and so the behaviour
is indeed undefined.

Right now I can't think of a sane way to fix this.

Comment 1

[Julian Seward [:jseward]]

One solution that does keep Ubsan happy is to iterate using an integer
value, and only form the Side value inside the loop body:

#define NS_FOR_CSS_SIDES(var_) \
   for (int var_##_int = (int)NS_SIDE_TOP; \
            var_##_int <= (int)NS_SIDE_LEFT; var_##_int++) { \
      mozilla::css::Side var_ = mozilla::css::Side(var_##_int);

The problem is that this gives an extra { that needs to be gotten
rid of somehow.  For experimentation I manually removed the { after
all circa 70 uses of this macro just to see if the fix would work.

This is ugly, makes the macro inconsistent with all the other NS_FOR_*
macros, and causes the source files to have unbalanced { and }s, which
confuses and slows down emacs greatly.  So I'm not proposing this as a
landable fix.

That said, I would like to get this fixed somehow.  For a startup and
quit of the browser, this far and away the most commonly reported UBSan
complaint.

Comment 2

[Julian Seward [:jseward]]

Attachment: Hideous hack as per comment #1, do not land!

Comment 3

[Mats Palmgren (inactive)]

Maybe we could use a helper class that implicitly converts to Side?

#define NS_FOR_CSS_SIDES(var_) for (mozilla::css::SideLoopHelper var_; var_.i <= NS_SIDE_LEFT; ++var_.i)

and add in mozilla::css::

struct SideLoopHelper {
  SideLoopHelper() : i(NS_SIDE_TOP) {}
  operator Side() {
    MOZ_ASSERT(i >= NS_SIDE_TOP && i <= NS_SIDE_LEFT, "Out of range side");
    return Side(i);
  }
  explicit operator Corner() {
    return Corner(Side(i));
  }
  int32_t i;
};

It's a bit ugly, but we don't need to change any other code.

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1299379-2.cset

(In reply to Mats Palmgren (:mats) from comment #3)
> Maybe we could use a helper class that implicitly converts to Side?

Ah, nice suggestion.  Here's a patch that does that, and it does indeed
keep Ubsan happy.  Compared to your suggestion:

* the new class is called SideIterHelper

* I added a start-value for the constructor, so as to avoid the
  asymmetry that the end value is visible in the #define but the
  start value isn't.

* I added some comments.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8787140 [details] [diff] [review]
bug1299379-2.cset

>+  SideIterHelper(Side first_value) : i(first_value) {}

Nit: camelCase and snake_case looks a bit odd on the same line.
I suggest aFirstValue instead.

Otherwise, it looks fine to me.

Comment 6

[Julian Seward [:jseward]]

(In reply to Mats Palmgren (:mats) from comment #5)
Thanks!  Ok, I'll fix the nit and get it tested.  Would you be
so kind as to upgrade the f+ to r+ so I can land it?

Comment 7

[Julian Seward [:jseward]]

Hmm, the patch doesn't build on Windows, due to errors like this:

z:/task_1473089207/build/src/layout/generic/nsFrame.cpp(1301): error C2593: 'operator +' is ambiguous
z:/task_1473089207/build/src/layout/generic/nsFrame.cpp(1301): error C2088: '+': illegal for struct
z:/task_1473089207/build/src/layout/generic/nsFrame.cpp(1302): error C2593: 'operator +' is ambiguous
z:/task_1473089207/build/src/layout/generic/nsFrame.cpp(1302): error C2088: '+': illegal for struct
z:/task_1473089207/build/src/layout/generic/nsFrame.cpp(1304): error C2593: 'operator %' is ambiguous

My C++-fu isn't good enough to know why.  I presume that the 'operator +' it is referring to
is to do with the "++var_.i" in the proposed new NS_FOR_CSS_SIDES macro, but apart from that
I'm just guessing.  Any suggestions?

Comment 8

[Mats Palmgren (inactive)]

Attachment: addendum

It looks like the errors comes from these macros, which use + and % on
"side_" and an integer value.  I think adding an explicit Side(side_)
should fix it.

If there are other sites that does arithmetic like this then maybe
we should try adding operator+(int32_t) etc to SideIterHelper instead...

Comment 9

[Nicholas Nethercote [inactive]]

What about simply adding a |eSideLimit| value to the Side enum? There are heaps of other enums in the codebase that have a limit value for this purpose.

Comment 10

[Mats Palmgren (inactive)]

We have many switch-statements for these values so that would give a warning there instead.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Alternative fix using builtin iterator

This seems to work fine locally for me with clang 3.8.0 on Linux.
I pushed it to Try to see if works elsewhere:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37f9a633d51e

If it does, then we should probably just replace all NS_FOR_CSS_SIDES
with this for-loop since it would make the code clearer.

Comment 12

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

Comment on attachment 8788514 [details] [diff] [review]
Alternative fix using builtin iterator

Does it compile to the same thing with gcc, clang, and MSVC?

Comment 13

[Julian Seward [:jseward]]

(In reply to Mats Palmgren (:mats) from comment #11)
> Created attachment 8788514 [details] [diff] [review]
> Alternative fix using builtin iterator

I would prefer this if it works.  The previous scheme of having a SideIterHelper
struct is proving difficult to get compiling with MSVC.

Comment 14

[Julian Seward [:jseward]]

(In reply to Mats Palmgren (:mats) from comment #11)
> Created attachment 8788514 [details] [diff] [review]
> Alternative fix using builtin iterator

This keeps Ubsan happy, so +1 from my point of view.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Use builtin array iterator instead of NS_FOR_CSS_SIDES macro

Comment 16

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f81676eb6d2

Comment 17

[Mats Palmgren (inactive)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #12)
> Does it compile to the same thing with gcc, clang, and MSVC?

I'm not sure what you're asking.  Are you worried about the performance of the new code
and want to know if the compiler can optimize it to an equivalent iteration?
If so, the answer is no.  At least with with clang 3.8.0 on Linux it will do a memory
read from kAllSides to get the index (Side).

Or are you worried about compatibility issues between compilers for range-based
for-loops?  Or something else?

Comment 18

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

(In reply to Mats Palmgren (:mats) from comment #17)
> I'm not sure what you're asking.  Are you worried about the performance of
> the new code
> and want to know if the compiler can optimize it to an equivalent iteration?
> If so, the answer is no.  At least with with clang 3.8.0 on Linux it will do
> a memory
> read from kAllSides to get the index (Side).

Yes, that's what I was worried about.

And given that response I don't think we should land this patch.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8788830 [details] [diff] [review]
Use builtin array iterator instead of NS_FOR_CSS_SIDES macro

OK, I guess this is too slow then.
FTR, since the whole kAllSides fits in one cache line I think we could
make it one slow read at the most.  From a static address so there is
some room for ILP there.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Alt. 3

Julian, does this fix the ubsan warning?

Comment 21

[Julian Seward [:jseward]]

(In reply to Mats Palmgren (:mats) from comment #20)
> Created attachment 8788993 [details] [diff] [review]
> Julian, does this fix the ubsan warning?

Yes.  It does.

Comment 22

[Mats Palmgren (inactive)]

Attachment: fix, v3

We have a dozen or so token pasting macros in various places so
I think we should have a blessed one somewhere in mfbt instead.

The assembler code with/without the patch is identical in an Opt
build using clang 3.8.0 on Linux.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44bab96d90d8

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 8789421 [details] [diff] [review]
fix, v3

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

I'm neither a layout/ nor an mfbt/ peer, and I already r+'d one patch in this bug that dbaron subsequently r-'d. So I will defer review here, sorry.

Having said that, some comments:

- Yikes! Sad that it requires something this ugly :(  Esp. the MOZ_CONCAT(var_,__LINE__) name.

- Having to declare a variable outside the loop is unfortunate. I guess it's because you can't declare two variables of different types within a loop header?

- It'd be nice to remove some of the other pasting macros at the same time, or as a follow-up.

Comment 24

[Mats Palmgren (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #23)
> - Yikes! Sad that it requires something this ugly :(  Esp. the
> MOZ_CONCAT(var_,__LINE__) name.

Yeah, it's ugly for sure.

> - Having to declare a variable outside the loop is unfortunate. I guess it's
> because you can't declare two variables of different types within a loop
> header?

Yes.

> - It'd be nice to remove some of the other pasting macros at the same time,
> or as a follow-up.

I'll file a follow-up bug(s).

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8789421 [details] [diff] [review]
fix, v3

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

dbaron should probably review this, since he's already expressed some strong opinions about options here -- and this is a (necessarily) ugly enough hack that I'd rather he make the call, in his role as module owner. :)

And I think this needs an MFBT reviewer (Waldo, maybe?) for the MacroArgs change. (I'll also admit to being unfamiliar-enough with macro arcana that I don't entirely understand the subtlety behind & need for MOZ_CONCAT/MOZ_CONCAT2, too.)

::: layout/style/nsStyleConsts.h
@@ +19,5 @@
>  typedef mozilla::Side Side;
>  } // namespace css
>  
> +#define NS_FOR_CSS_SIDES(var_)                                           \
> +  int32_t MOZ_CONCAT(var_,__LINE__) = NS_SIDE_TOP;                       \

This #define needs some documentation to explain what it's doing, since it seems pretty arcane.

Both at a high level (e.g. "creates a for loop that walks over the four mozilla::css::Side values"), and also explaining the idea behind & reason for the somewhat-arcane implementation here.

@@ +22,5 @@
> +#define NS_FOR_CSS_SIDES(var_)                                           \
> +  int32_t MOZ_CONCAT(var_,__LINE__) = NS_SIDE_TOP;                       \
> +  for (mozilla::css::Side var_;                                          \
> +       MOZ_CONCAT(var_,__LINE__) <= NS_SIDE_LEFT &&                      \
> +         ((var_ = mozilla::css::Side(MOZ_CONCAT(var_,__LINE__))), true); \

What is the ", true" doing here? I'm not clear on where/how that comes into play in this expression.

Comment 26

[Mats Palmgren (inactive)]

Attachment: Add MOZ_CONCAT macro in mfbt

I figured we should have a common definition of this macro somewhere in mfbt
so we don't have to define it over and over again when needed.

Comment 27

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> dbaron should probably review this, since he's already expressed some strong
> opinions about options here -- and this is a (necessarily) ugly enough hack
> that I'd rather he make the call, in his role as module owner. :)

"David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) <[e-mail]> is not currently accepting 'review' requests."

Comment 28

[Mats Palmgren (inactive)]

Attachment: fix

This uses an int32_t loop variable to avoid the UBSan warning.
The value of that is then assigned to "var_" in the 2nd term
of the && expr, hence the assignment only occurs for the valid
Side values.  The ",true" part is a comma expression to make
the 2nd term not affect the loop condition, so the 2nd term
effectively acts as an assignment inside the loop.

The assembler code with/without the patch is identical in an Opt
build using clang 3.8.0 on Linux.

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8790022 [details] [diff] [review]
fix

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

OK, r=me I suppose -- though remember to switch the commit message back to "r=dholbert" (not dbaron) before landing. :)

::: layout/style/nsStyleConsts.h
@@ +20,5 @@
>  } // namespace css
>  
> +// Creates a for loop that walks over the four mozilla::css::Side values.
> +// We use an int32_t help variable to avoid causing an UBSan warning
> +// (see bug 1299379).

Thanks for adding this comment!  Though, hmm, this is perhaps still a bit vague about what the problem is & why we're solving it this way. Maybe replace the last 2 lines here with this slightly-clearer (IMO) version (or something like it), which leaves fewer questions in the reader's mind:

// We use an int32_t helper variable (instead of a Side) for our loop counter,
// to avoid triggering undefined behavior just before we exit the loop (at
// which point the counter is incremented beyond the largest valid Side value).

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #28)
> The assembler code with/without the patch is identical in an Opt
> build using clang 3.8.0 on Linux.

(Thank you for verifying that! That's I think the prime thing that dbaron was concerned with here -- the assembler code staying at the same level of complexity/expensiveness. So, this verification makes me feel more comfortable r+'ing this without his input on this final formulation of the fix.)

Comment 31

[Jeff Walden [:Waldo]]

Comment on attachment 8790019 [details] [diff] [review]
Add MOZ_CONCAT macro in mfbt

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

::: mfbt/MacroArgs.h
@@ +12,5 @@
>  #define mozilla_MacroArgs_h
>  
> +// Concatenates pre-processor tokens in a way that can be used with __LINE__.
> +#define MOZ_CONCAT2(x,y) x##y
> +#define MOZ_CONCAT(x,y) MOZ_CONCAT2(x,y)

Space after all the commas, spaces around ##.

I'm not wholly convinced this is the best place for this, at least not with line 8's description, but good enough for now.

Comment 32

[Jeff Walden [:Waldo]]

Prior r+ aside, and to return to this bug's original issue.  Given some of the shadowing/bracing horrors discussed in comments, requiring tiptoeing around at length in proposed solutions, why are we still using a macro here?

C++11 added lambda expressions.  We have full compiler support for them everywhere.  js/src uses a boatload of them, especially in the frontend.  They exist, they're reliable, you can use them.

This seems like a case tailor-made for a higher-order function:

template<typename ForCssSide>
static inline void
ForAllCssSides(ForCssSide forSide)
{
  // Or do int+casting to enumerate the sides.  It doesn't matter.
  static constexpr allSides[] = { eSideTop, eSideRight, eSideBottom, eSideLeft };
  for (css::Side side : allSides)
    forSide(side);
}

An example like this:

    NS_FOR_CSS_SIDES(side) {
      // Clamp negative calc() to 0.
      aPadding.Side(side) = std::max(mPadding.ToLength(side), 0);
    }

would simply convert to this:

    ForAllCssSides([&aPadding, this](css::Side side) {
      // Clamp negative calc() to 0.
      aPadding.Side(side) = std::Max(mPadding.ToLength(side), 0);
    });

Skimming uses, there are a few uses this wouldn't support -- those that really want to early-exit.  It *appears* all of those are really asking, "is this property true of all sides?"  So you could have an alternate function for those:

template<typename Predicate>
static inline bool
TestAllCssSides(Predicate pred)
{
  // Or do int+casting to enumerate the sides.  It doesn't matter.
  static constexpr allSides[] = { eSideTop, eSideRight, eSideBottom, eSideLeft };
  for (css::Side side : allSides) {
    if (!pred(side))
      return false;
  }
  return true;
}

And then this:

  NS_FOR_CSS_SIDES(ix) {
    if (mBorderStyle[ix] != aNewData.mBorderStyle[ix] ||
        mBorderColor[ix] != aNewData.mBorderColor[ix]) {
      return nsChangeHint_RepaintFrame;
    }
  }


would transform into this (feel free to inline the lambda into its one use if desired):

  auto borderRemainsSame = [this, &aNewData](css::Side side) {
    return mBorderStyle[side] == aNewData.mBorderStyle[side] &&
           mBorderColor[side] == aNewData.mBorderColor[side];
  };
  if (!TestAllSides(borderRemainsSame))
    return nsChangeHint_RepaintFrame;

Macros for NS_FOR_CSS_SIDES's purposes are anachronistic.  And by not using them, you avoid a bunch of the shadowing discussions in this bug, you don't have extra braces, &c.  You should use lambdas for this now!

Comment 33

[Mats Palmgren (inactive)]

I suggested using an array + builtin iterator in attachment 8788830 [details] [diff] [review].
Unfortunately, it was deemed too slow, per comment 18.

FTR, I did check the optimized assembler for the ForAllCssSides above;
it reads the Side values through a pointer, so has the same problem.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d16d24f4db
Add MOZ_CONCAT for preprocessor token pasting.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa475defd61c
Iterate Sides using an int32_t instead to avoid UBSan warning.  r=dholbert

Comment 35

[Nicholas Nethercote [inactive]]

Thank you for perservering with this trickier-than-expected bug, Mats. It'll be a big help with Julian's UBSan investigations, which is a good thing.

Comment 36

[Jeff Walden [:Waldo]]

I should say also that it *should* be a full and complete fix to add a final enum _LIMIT value.  C++11's description of the values in an enum type says basically it's the smallest N-bit integer sufficient to hold all the enumerators.  Here those are 0,1,2,3 -- so simply adding a 4 would make values 0-7 valid and would mean operator+ wouldn't create an invalid Side value.  But I don't know whether code is switching on Sides such that adding another initializer would be undesirable.

Comment 37

[Nicholas Nethercote [inactive]]

> But I don't know whether code is switching on Sides such that adding
> another initializer would be undesirable.

Mats said that in comment 10. I took a look and there weren't that many switches -- a dozen or so -- and some already had code to handle out-of-bad values. But, eh, it didn't seem that important.

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/64d16d24f4db
https://hg.mozilla.org/mozilla-central/rev/fa475defd61c