1427229 - Perform validation when sending an EnumSet over IPC

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Botond Ballo [:botond] [back on May 21]]

As discussed in bug 1425926 comment 2, it would be good to perform validation when sending an EnumSet<T> over IPC.

Here is one way this could be done:

  - Define a trait, called "MaxEnumValue" or similar, that
    takes an enumeration type "T" and returns the largest
    valid value for it.

     - The trait would have no default implementation.
       It would need to be specialized for each enumeration 
       type T for which EnumSet<T> is sent over IPC.

  - ParamTraits<EnumSet<T>> would use MaxEnumValue<T> to
    construct an "ALL_BITS" value, an integer value where 
    all bits from 0 through MaxEnumValue<T>, inclusive,
    are set.

  - In its Read() and Write() methods, ParamTraits<EnumSet<T>>
    would validate the value being read/written against the
    ALL_BITS value, similar to how BitFlagsEnumValidator
    does it [1].

[1] https://searchfox.org/mozilla-central/rev/8ed13df845595dcb50c381aadde678706fd22332/ipc/glue/IPCMessageUtils.h#183

Comment 1

[zielas.daniel]

I would like to take care of this improvement.

Comment 2

[Botond Ballo [:botond] [back on May 21]]

Great! I assigned the bug to you.

I think comment 0 should contain enough details to get started. Let me know if you have any questions!

Comment 3

[zielas.daniel]

Attachment: bug_1427229.patch

Happy New Year!

I have attached the patch with the validation. Please check if way of solving the issue is aligned with your idea.

Comment 4

[Botond Ballo [:botond] [back on May 21]]

Comment on attachment 8941566 [details] [diff] [review]
bug_1427229.patch

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

Thanks, this looks pretty good!

My main suggestion is that we change MaxEnumValue to be at namespace scope rather than nested inside ParamTraits. That way, it doesn't depend on ParamTraits (I can envision wanting to know the max value in contexts outside of IPC), and it's easier to specialize. I would also suggest expressing the trait as a class rather than a function; in addition to being more conventional, this allows the trait to be partially or explicitly specialized (whereas functions can only be explicitly specialized).

That would look something like this:

  // Trait definition

  // At namespace scope
  template <typename T>
  struct MaxEnumValue;  // no need to define the primary template

  // Example specialization

  enum ExampleEnum
  {
    CAT = 0,
    DOG,
    HAMSTER
  };
  template <>
  struct MaxEnumValue<ExampleEnum>
  {
    static constexpr int value = HAMSTER;
  };

As for where to define the trait, I think it can go into a more general place than IPCMessageUtils.h, such as mfbt/TypeTraits.h.

::: ipc/glue/IPCMessageUtils.h
@@ +963,5 @@
> +
> +  template<typename U>
> +  static constexpr int MaxEnumValue();
> +
> +  static constexpr uint32_t AllEnumBits()

We should probably use the same type here and for the type of the 'tmp' variable in Read().

We can make a class-scope typedef to avoid repetition:

  typedef uint32_t serializedType;

Or, if you prefer not to assume it's a uint32_t but use "the type returned by serialize()" as we currently do for 'tmp':

  typedef decltype(result->serialize()) serializedType;

(Oh wait, we don't have "result" at class scope. DeclVal [1] to the rescue:

  typedef decltype(DeclVal<paramType*>()->serialize()) serializedType;

)

[1] https://searchfox.org/mozilla-central/source/mfbt/TypeTraits.h#38

@@ +971,5 @@
> +
> +  static constexpr bool IsLegalValue(const uint32_t value)
> +  {
> +    MOZ_STATIC_ASSERT(MaxEnumValue<T>() >= 0 &&
> +                      MaxEnumValue<T>() < std::numeric_limits<uint32_t>::digits,

#include <limits> for std::numeric_limits

Comment 5

[zielas.daniel]

Hello,

Thanks for the feedback.

1) I have prepared changes related with exchanging MaxEnumValue function into struct and moving it to TrypeTraits. Nevertheless I would like to confirm one assumption, specializations of 'struct MaxEnumValue' will be placed in headers where each Enum is defined, yes?

2) According to 'serializedType', what do you think about adding a 'typedef uint32_t serializedType' into 'EnumSet' class (similar like 'std::vector<T>' has 'value_type')? Together with this change update API of 'EnumSet' just to be consistent:
'uint32_t serialize() const' -> 'serializedType serialize() const'
'void deserialize(uint32_t aValue)' -> 'void deserialize(serializedType aValue)'

And then will be possible to use 'serializedType' in more traditional way 'typename EnumSet<T>::serializedType tmp' and 'static constexpr typename mozilla::EnumSet<T>::serializedType AllEnumBits() { ...'.

3) And one question which is coming back to me all the time, why is created mozilla::EnumSet from scratch instead of reusing std::bitset?

Comment 6

[Botond Ballo [:botond] [back on May 21]]

(In reply to szefoski22 from comment #5)
> 1) I have prepared changes related with exchanging MaxEnumValue function
> into struct and moving it to TrypeTraits. Nevertheless I would like to
> confirm one assumption, specializations of 'struct MaxEnumValue' will be
> placed in headers where each Enum is defined, yes?

Yes.

> 2) According to 'serializedType', what do you think about adding a 'typedef
> uint32_t serializedType' into 'EnumSet' class (similar like 'std::vector<T>'
> has 'value_type')? Together with this change update API of 'EnumSet' just to
> be consistent:
> 'uint32_t serialize() const' -> 'serializedType serialize() const'
> 'void deserialize(uint32_t aValue)' -> 'void deserialize(serializedType
> aValue)'
> 
> And then will be possible to use 'serializedType' in more traditional way
> 'typename EnumSet<T>::serializedType tmp' and 'static constexpr typename
> mozilla::EnumSet<T>::serializedType AllEnumBits() { ...'.

Yes, that sounds like a nice improvement!

> 3) And one question which is coming back to me all the time, why is created
> mozilla::EnumSet from scratch instead of reusing std::bitset?

std::bitset does not encode the type of the enumerator representing the individual bits. This has several implications:

  - You could accidentally mix up a std::bitset representing
    values from EnumA and one representing values from EnumB.
    With EnumSet, the compiler catches this because they have
    different types (EnumSet<EnumA> vs. EnumSet<EnumB>).

  - Adding an enumerator to a std::bitset would require a
    cast to an integer at the call site if the enum type is
    an "enum class" (which does not implicitly convert to
    integer).

  - You couldn't do things like iterate the std::bitset via
    begin() and end(), and get values of the enumeration type
    back, the way you can with EnumSet.

There may be other reasons I'm not thinking of as well, that's just what comes to mind right now.

Comment 7

[zielas.daniel]

Attachment: bug_1427229_1.patch

Please check this patch.

Comment 8

[Botond Ballo [:botond] [back on May 21]]

Comment on attachment 8944528 [details] [diff] [review]
bug_1427229_1.patch

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

Looks good, thanks! Just one minor comment.

I'm also going to flag :froydnj for review on the mfbt changes.

::: ipc/glue/IPCMessageUtils.h
@@ +945,4 @@
>      return false;
>    }
> +
> +  static constexpr typename mozilla::EnumSet<T>::serializedType AllEnumBits()

We can avoid repetition of "typename mozilla::EnumSet<T>::serializedType" several times with a class-scope typedef:

  typedef typename mozilla::EnumSet<T>::serializedType serializedType;

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8944528 [details] [diff] [review]
bug_1427229_1.patch

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

This seems pretty reasonable, just a few things that need to be fixed below.

::: ipc/glue/IPCMessageUtils.h
@@ +947,5 @@
> +
> +  static constexpr typename mozilla::EnumSet<T>::serializedType AllEnumBits()
> +  {
> +    return static_cast<typename mozilla::EnumSet<T>::serializedType>
> +      ((1UL << (mozilla::MaxEnumValue<T>::value + 1)) - 1);

What happens here when MaxEnumValue<T>::value is 31 and 1UL is just 32 bits, rather than 64 bits?  Then we are shifting by 32, which isn't valid.  I can see from evaluating (1UL << 32) - 1 with gcc and clang that the result depends on whether optimization is enabled, which isn't good, and both compilers warn about such code.

So we should either handle 31 as a special case, or we should write a more general expression that would handle 31 cleanly.

@@ +952,5 @@
> +  }
> +
> +  static constexpr bool IsLegalValue(const typename mozilla::EnumSet<T>::serializedType value)
> +  {
> +    MOZ_STATIC_ASSERT(mozilla::MaxEnumValue<T>::value >= 0 &&

Please use static_assert in C++ code instead of MOZ_STATIC_ASSERT.

Should we require that MaxEnumValue<T>::value is an unsigned int, rather than an int?  That would get us the checking for >= 0 "for free", and would also be more reflective of values that can be contained in EnumSet<T> types.

Either way, we should static_assert on the signedness of MaxEnumValue<T>::value to ensure that users define it consistently.

::: mfbt/TypeTraits.h
@@ +1329,5 @@
> + *     static constexpr int value = static_cast<int>(HAMSTER);
> + *   };
> + */
> +template <typename T>
> +struct MaxEnumValue;  // no need to define the primary template

TypeTraits.h is intended to be a stand-in for <type_traits>, and as such, we prefer to not define traits in TypeTraits.h that aren't defined in <type_traits>.  This class should be moved, probably to EnumSet.h...though moving it to IPCMessageUtils.h would make it a little clearer that you need to specialize it if you're sending EnumSet<T> over IPC.  What do you think?

Comment 10

[Botond Ballo [:botond] [back on May 21]]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> ::: mfbt/TypeTraits.h
> @@ +1329,5 @@
> > + *     static constexpr int value = static_cast<int>(HAMSTER);
> > + *   };
> > + */
> > +template <typename T>
> > +struct MaxEnumValue;  // no need to define the primary template
> 
> TypeTraits.h is intended to be a stand-in for <type_traits>, and as such, we
> prefer to not define traits in TypeTraits.h that aren't defined in
> <type_traits>.  This class should be moved, probably to EnumSet.h...though
> moving it to IPCMessageUtils.h would make it a little clearer that you need
> to specialize it if you're sending EnumSet<T> over IPC.  What do you think?

I think MaxEnumValue has the potential to be used for things unrelated to IPC or EnumSets, so putting it in a more general header would be preferable. After discussion with Nathan, it seems mfbt/EnumTypeTraits.h [1] would be a good place.

Comment 11

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #8)

Thanks for the review!

> Comment on attachment 8944528 [details] [diff] [review]
> bug_1427229_1.patch
> 
> Review of attachment 8944528 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks! Just one minor comment.
> 
> I'm also going to flag :froydnj for review on the mfbt changes.
> 
> ::: ipc/glue/IPCMessageUtils.h
> @@ +945,4 @@
> >      return false;
> >    }
> > +
> > +  static constexpr typename mozilla::EnumSet<T>::serializedType AllEnumBits()
> 
> We can avoid repetition of "typename mozilla::EnumSet<T>::serializedType"
> several times with a class-scope typedef:
> 
>   typedef typename mozilla::EnumSet<T>::serializedType serializedType;

Fully Agree.



(In reply to Nathan Froyd [:froydnj] from comment #9)

Thanks for the review!

> Comment on attachment 8944528 [details] [diff] [review]
> bug_1427229_1.patch
> 
> Review of attachment 8944528 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems pretty reasonable, just a few things that need to be fixed below.
> 
> ::: ipc/glue/IPCMessageUtils.h
> @@ +947,5 @@
> > +
> > +  static constexpr typename mozilla::EnumSet<T>::serializedType AllEnumBits()
> > +  {
> > +    return static_cast<typename mozilla::EnumSet<T>::serializedType>
> > +      ((1UL << (mozilla::MaxEnumValue<T>::value + 1)) - 1);
> 
> What happens here when MaxEnumValue<T>::value is 31 and 1UL is just 32 bits,
> rather than 64 bits?  Then we are shifting by 32, which isn't valid.  I can
> see from evaluating (1UL << 32) - 1 with gcc and clang that the result
> depends on whether optimization is enabled, which isn't good, and both
> compilers warn about such code.
> 
> So we should either handle 31 as a special case, or we should write a more
> general expression that would handle 31 cleanly.

Fully agree.


> @@ +952,5 @@
> > +  }
> > +
> > +  static constexpr bool IsLegalValue(const typename mozilla::EnumSet<T>::serializedType value)
> > +  {
> > +    MOZ_STATIC_ASSERT(mozilla::MaxEnumValue<T>::value >= 0 &&
> 
> Please use static_assert in C++ code instead of MOZ_STATIC_ASSERT.

OK

 
> Should we require that MaxEnumValue<T>::value is an unsigned int, rather
> than an int?  That would get us the checking for >= 0 "for free", and would
> also be more reflective of values that can be contained in EnumSet<T> types.
> 
> Either way, we should static_assert on the signedness of
> MaxEnumValue<T>::value to ensure that users define it consistently.

Sure, the additional assert will guard this fine.



(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Nathan Froyd [:froydnj] from comment #9)
> > ::: mfbt/TypeTraits.h
> > @@ +1329,5 @@
> > > + *     static constexpr int value = static_cast<int>(HAMSTER);
> > > + *   };
> > > + */
> > > +template <typename T>
> > > +struct MaxEnumValue;  // no need to define the primary template
> > 
> > TypeTraits.h is intended to be a stand-in for <type_traits>, and as such, we
> > prefer to not define traits in TypeTraits.h that aren't defined in
> > <type_traits>.  This class should be moved, probably to EnumSet.h...though
> > moving it to IPCMessageUtils.h would make it a little clearer that you need
> > to specialize it if you're sending EnumSet<T> over IPC.  What do you think?
> 
> I think MaxEnumValue has the potential to be used for things unrelated to
> IPC or EnumSets, so putting it in a more general header would be preferable.
> After discussion with Nathan, it seems mfbt/EnumTypeTraits.h [1] would be a
> good place.

OK

Comment 12

[zielas.daniel]

Attachment: bug_1427229_2.patch

Please take a look on this one.

Comment 13

[zielas.daniel]

Attachment: bug_1427229_3.patch

Comment 14

[zielas.daniel]

Attachment: bug_1427229_4.patch

Comment 15

[Botond Ballo [:botond] [back on May 21]]

Comment on attachment 8945625 [details] [diff] [review]
bug_1427229_4.patch

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

Looks good to me, thanks!

Comment 16

[Botond Ballo [:botond] [back on May 21]]

Comment on attachment 8945625 [details] [diff] [review]
bug_1427229_4.patch

(Note for future reference: there is a 'review' flag that you can set to '?' on a patch to request review from someone. We generally prefer that to 'needinfo' for requesting review on a patch, because the 'review' flag is specific to the patch.)

Comment 17

[Botond Ballo [:botond] [back on May 21]]

I pushed the patch to our Try server to make sure it's building on all our platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=445f4ecf7ac12a65a9f306f50d37b7df1b8a720f

Comment 18

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #16)
> Comment on attachment 8945625 [details] [diff] [review]
> bug_1427229_4.patch
> 
> (Note for future reference: there is a 'review' flag that you can set to '?'
> on a patch to request review from someone. We generally prefer that to
> 'needinfo' for requesting review on a patch, because the 'review' flag is
> specific to the patch.)

OK, I will remember, thanks!

Comment 19

[Botond Ballo [:botond] [back on May 21]]

(In reply to Botond Ballo [:botond] from comment #17)
> I pushed the patch to our Try server to make sure it's building on all our
> platforms:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=445f4ecf7ac12a65a9f306f50d37b7df1b8a720f

This is looking good.

Comment 20

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #19)
> (In reply to Botond Ballo [:botond] from comment #17)
> > I pushed the patch to our Try server to make sure it's building on all our
> > platforms:
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=445f4ecf7ac12a65a9f306f50d37b7df1b8a720f
> 
> This is looking good.

Great :)

Comment 21

[zielas.daniel]

Attachment: bug_1427229_5.patch

I believe this is the last update. I have found a typo in the example and I have noticed that static_cast in AllEnumBits function is pointless.

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8946089 [details] [diff] [review]
bug_1427229_5.patch

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

Looks good to me, just one very small thing to fix.

::: ipc/glue/IPCMessageUtils.h
@@ +956,5 @@
> +  static constexpr bool IsLegalValue(const serializedType value)
> +  {
> +    static_assert(mozilla::MaxEnumValue<T>::value < std::numeric_limits<serializedType>::digits,
> +                  "Enum max value is not in the range!");
> +    static_assert(std::is_unsigned<decltype(mozilla::MaxEnumValue<T>::value)>::value,

Nit: since std::is_unsigned comes from <type_traits>, that header should be included at the top of this file.

Comment 23

[Botond Ballo [:botond] [back on May 21]]

Comment on attachment 8946089 [details] [diff] [review]
bug_1427229_5.patch

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

Looks good to me as well! If you could upload one more version with Nathan's comment fixed, I will go ahead and commit it.

Comment 24

[zielas.daniel]

Attachment: bug_1427229_6.patch

Comment 25

[Botond Ballo [:botond] [back on May 21]]

Comment on attachment 8946406 [details] [diff] [review]
bug_1427229_6.patch

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

Great, thanks! I'll go ahead and carry the r+ for Nathan as well.

Comment 26

[Botond Ballo [:botond] [back on May 21]]

I changed the end of the commit message to "r=botond,froydnj" to reflect the fact that Nathan also reviewed the patch, and committed it to mozilla-inbound. There it will undergo some more automated testing (you can see the progress here [1]) and should then get merged into mozilla-central within a day or so.

Comment 27

[Botond Ballo [:botond] [back on May 21]]

Whoops, forgot the link:

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1810b8f69bf358c3bf6c89d20d2a54b17cb8839f

Comment 28

[zielas.daniel]

Thanks!

Comment 29

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1810b8f69bf3

Comment 30

[Botond Ballo [:botond] [back on May 21]]

Thanks for your work here, Daniel!

Let me know if you're interested in working on another bug and would like some suggestions.

Comment 31

[zielas.daniel]

Hi, Yes, I'm interested to take something.

Comment 32

[Botond Ballo [:botond] [back on May 21]]

(In reply to szefoski22 from comment #31)
> Hi, Yes, I'm interested to take something.

Here are a couple of other bugs I'm mentoring that you might be interested in:

  - Bug 1434710. This is a straightforward refactoring to remove
    a homegrow utility class, mozilla::IndexSequence, and replace
    it with its standard library equivalent, std::index_sequence.

  - Bug 1420512. This is a bit more involved refactoring to
    remove some duplication in data structures used in the
    Layers API (part of Firefox's rendering pipeline).

If either of those sound interesting to you, please comment in the respective bug and I'll provide more details there.

There is also a tool for searching for mentored bugs that you may want to have a look at:

https://www.joshmatthews.net/bugsahoy/