Perform validation when sending an EnumSet over IPC

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: botond, Assigned: zielas.daniel, Mentored)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

2 years ago
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
Assignee

Comment 1

2 years ago
I would like to take care of this improvement.
Reporter

Comment 2

2 years ago
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!
Assignee: nobody → szefoski22
Priority: -- → P3
Assignee

Comment 3

Last year
Posted patch bug_1427229.patch (obsolete) — Splinter Review
Happy New Year!

I have attached the patch with the validation. Please check if way of solving the issue is aligned with your idea.
Flags: needinfo?(botond)
Reporter

Comment 4

Last year
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
Attachment #8941566 - Flags: feedback+
Reporter

Updated

Last year
Flags: needinfo?(botond)
Assignee

Comment 5

Last year
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?
Flags: needinfo?(botond)
Reporter

Comment 6

Last year
(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.
Flags: needinfo?(botond)
Assignee

Comment 7

Last year
Posted patch bug_1427229_1.patch (obsolete) — Splinter Review
Please check this patch.
Flags: needinfo?(botond)
Reporter

Updated

Last year
Attachment #8941566 - Attachment is obsolete: true
Flags: needinfo?(botond)
Reporter

Comment 8

Last year
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;
Attachment #8944528 - Flags: review?(nfroyd)
Attachment #8944528 - Flags: feedback+
Reporter

Updated

Last year
Depends on: 1425926
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?
Attachment #8944528 - Flags: review?(nfroyd) → feedback+
Reporter

Comment 10

Last year
(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.
Assignee

Comment 11

Last year
(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
Assignee

Comment 12

Last year
Posted patch bug_1427229_2.patch (obsolete) — Splinter Review
Please take a look on this one.
Attachment #8944528 - Attachment is obsolete: true
Flags: needinfo?(botond)
Assignee

Updated

Last year
Flags: needinfo?(nfroyd)
Assignee

Comment 13

Last year
Posted patch bug_1427229_3.patch (obsolete) — Splinter Review
Attachment #8945617 - Attachment is obsolete: true
Assignee

Comment 14

Last year
Posted patch bug_1427229_4.patch (obsolete) — Splinter Review
Attachment #8945620 - Attachment is obsolete: true
Reporter

Comment 15

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

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

Looks good to me, thanks!
Attachment #8945625 - Flags: review+
Reporter

Comment 16

Last year
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.)
Flags: needinfo?(nfroyd)
Flags: needinfo?(botond)
Attachment #8945625 - Flags: review?(nfroyd)
Reporter

Comment 17

Last year
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
Assignee

Comment 18

Last year
(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!
Reporter

Comment 19

Last year
(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.
Assignee

Comment 20

Last year
(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 :)
Assignee

Comment 21

Last year
Posted patch bug_1427229_5.patch (obsolete) — Splinter Review
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.
Attachment #8945625 - Attachment is obsolete: true
Attachment #8945625 - Flags: review?(nfroyd)
Attachment #8946089 - Flags: review?(nfroyd)
Attachment #8946089 - Flags: review?(botond)
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.
Attachment #8946089 - Flags: review?(nfroyd) → review+
Reporter

Comment 23

Last year
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.
Attachment #8946089 - Flags: review?(botond) → review+
Assignee

Comment 24

Last year
Attachment #8946089 - Attachment is obsolete: true
Attachment #8946406 - Flags: review?(nfroyd)
Attachment #8946406 - Flags: review?(botond)
Reporter

Comment 25

Last year
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.
Attachment #8946406 - Flags: review?(nfroyd)
Attachment #8946406 - Flags: review?(botond)
Attachment #8946406 - Flags: review+
Reporter

Comment 26

Last year
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.
Assignee

Comment 28

Last year
Thanks!

Comment 29

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/1810b8f69bf3
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Reporter

Comment 30

Last year
Thanks for your work here, Daniel!

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

Comment 31

Last year
Hi, Yes, I'm interested to take something.
Flags: needinfo?(botond)
Reporter

Comment 32

Last year
(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/
Flags: needinfo?(botond)
Reporter

Updated

Last year
Blocks: 1420996
You need to log in before you can comment on or make changes to this bug.