1405142 - fsanitize=enum (ubsan) runtime errors for GtkStateFlags

Status: NEW

(Core :: Widget: Gtk, defect, P3)

Description

[Arthur Edelstein [:arthur]]

When I run automated tests of linux64-asan (clang) with -fsanitize=enum, I get the following errors:

 widget/gtk/WidgetStyleCache.cpp:1236:23 	 87 	 load of value 2048, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/WidgetStyleCache.cpp:1252:21 	 1698 	 load of value 128, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/WidgetStyleCache.cpp:1261:7 	 1698 	 load of value 128, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/gtk3drawing.cpp:1039:57 	 11 	 load of value 2048, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/gtk3drawing.cpp:1901:46 	 20 	 load of value 2048, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/gtk3drawing.cpp:1913:55 	 20 	 load of value 2048, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/gtk3drawing.cpp:343:50 	 2 	 load of value 2048, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/gtk3drawing.cpp:346:40 	 71 	 load of value 2048, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/gtk3drawing.cpp:506:42 	 77 	 load of value 128, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/gtk3drawing.cpp:508:41 	 77 	 load of value 128, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/nsLookAndFeel.cpp:184:37 	 1698 	 load of value 128, which is not a valid value for type 'GtkStateFlags'
 widget/gtk/nsLookAndFeel.cpp:204:50 	 1698 	 load of value 128, which is not a valid value for type 'GtkStateFlags'


Here's an example stack trace:

[task 2017-10-02T07:13:26.510Z] 07:13:26     INFO - GECKO(1092) |     #0 0x7fecfcad4582 in GetStyleContext(WidgetNodeType, GtkTextDir\
ection, GtkStateFlags, unsigned int) /builds/worker/workspace/build/src/widget/gtk/WidgetStyleCache.cpp:1252:21^M
[task 2017-10-02T07:13:26.528Z] 07:13:26     INFO - GECKO(1092) |     #1 0x7fecfcb1a598 in nsLookAndFeel::EnsureInit() /builds/worker\
/workspace/build/src/widget/gtk/nsLookAndFeel.cpp:832:13^M
[task 2017-10-02T07:13:26.530Z] 07:13:26     INFO - GECKO(1092) |     #2 0x7fecfcb1cd16 in nsLookAndFeel::NativeGetColor(mozilla::Loo\
kAndFeel::ColorID, unsigned int&) /builds/worker/workspace/build/src/widget/gtk/nsLookAndFeel.cpp:226:5^M
[task 2017-10-02T07:13:26.531Z] 07:13:26     INFO - GECKO(1092) |     #3 0x7fecfca883a1 in nsXPLookAndFeel::GetColorImpl(mozilla::Loo\
kAndFeel::ColorID, bool, unsigned int&) /builds/worker/workspace/build/src/widget/nsXPLookAndFeel.cpp:855:27^M
[task 2017-10-02T07:13:26.532Z] 07:13:26     INFO - GECKO(1092) |     #4 0x7fed0080e14c in nsWebBrowser::Create() /builds/worker/work\
space/build/src/toolkit/components/browser/nsWebBrowser.cpp:1210:3^M
[task 2017-10-02T07:13:26.564Z] 07:13:26     INFO - GECKO(1092) |     #5 0x7fecfc2ba9c0 in mozilla::dom::TabChild::Init() /builds/wor\
ker/workspace/build/src/dom/ipc/TabChild.cpp:586:15^M
[task 2017-10-02T07:13:26.570Z] 07:13:26     INFO - GECKO(1092) |     #6 0x7fecfc31f9aa in mozilla::dom::nsIContentChild::RecvPBrowse\
rConstructor(mozilla::dom::PBrowserChild*, mozilla::dom::IdType<mozilla::dom::TabParent> const&, mozilla::dom::IdType<mozilla::dom::T\
abParent> const&, mozilla::dom::IPCTabContext const&, unsigned int const&, mozilla::dom::IdType<mozilla::dom::ContentParent> const&, \
bool const&) /builds/worker/workspace/build/src/dom/ipc/nsIContentChild.cpp:97:7^M
[task 2017-10-02T07:13:26.587Z] 07:13:26     INFO - GECKO(1092) |     #7 0x7fecfc242fa3 in mozilla::dom::ContentChild::RecvPBrowserCo\
nstructor(mozilla::dom::PBrowserChild*, mozilla::dom::IdType<mozilla::dom::TabParent> const&, mozilla::dom::IdType<mozilla::dom::TabP\
arent> const&, mozilla::dom::IPCTabContext const&, unsigned int const&, mozilla::dom::IdType<mozilla::dom::ContentParent> const&, boo\
l const&) /builds/worker/workspace/build/src/dom/ipc/ContentChild.cpp:1839:27^M
[task 2017-10-02T07:13:26.623Z] 07:13:26     INFO - GECKO(1092) |     #8 0x7fecf7793abc in mozilla::dom::PContentChild::OnMessageRece\
ived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentChild.cpp:5025:20^M
[task 2017-10-02T07:13:26.640Z] 07:13:26     INFO - GECKO(1092) |     #9 0x7fecf6fefbeb in mozilla::ipc::MessageChannel::DispatchAsyn\
cMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2119:25^M
[task 2017-10-02T07:13:26.642Z] 07:13:26     INFO - GECKO(1092) |     #10 0x7fecf6fecaff in mozilla::ipc::MessageChannel::DispatchMes\
sage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2049:17^M

Comment 1

[Arthur Edelstein [:arthur]]

And one more representative stack trace:

[task 2017-10-02T07:26:41.501Z] 07:26:41     INFO - GECKO(1344) | /builds/worker/workspace/build/src/widget/gtk/nsLookAndFeel.cpp:204\
:50: runtime error: load of value 128, which is not a valid value for type 'GtkStateFlags'^M
[task 2017-10-02T07:26:41.503Z] 07:26:41     INFO - GECKO(1344) |     #0 0x7f6d3511ff0e in GetBorderColors /builds/worker/workspace/b\
uild/src/widget/gtk/nsLookAndFeel.cpp:204:50^M
[task 2017-10-02T07:26:41.507Z] 07:26:41     INFO - GECKO(1344) |     #1 0x7f6d3511ff0e in GetBorderColors(_GtkStyleContext*, unsigne\
d int*, unsigned int*) /builds/worker/workspace/build/src/widget/gtk/nsLookAndFeel.cpp:217^M
[task 2017-10-02T07:26:41.511Z] 07:26:41     INFO - GECKO(1344) |     #2 0x7f6d3511bdd8 in nsLookAndFeel::EnsureInit() /builds/worker\
/workspace/build/src/widget/gtk/nsLookAndFeel.cpp:986:9^M
[task 2017-10-02T07:26:41.513Z] 07:26:41     INFO - GECKO(1344) |     #3 0x7f6d3511cd16 in nsLookAndFeel::NativeGetColor(mozilla::Loo\
kAndFeel::ColorID, unsigned int&) /builds/worker/workspace/build/src/widget/gtk/nsLookAndFeel.cpp:226:5^M
[task 2017-10-02T07:26:41.518Z] 07:26:41     INFO - GECKO(1344) |     #4 0x7f6d350883a1 in nsXPLookAndFeel::GetColorImpl(mozilla::Loo\
kAndFeel::ColorID, bool, unsigned int&) /builds/worker/workspace/build/src/widget/nsXPLookAndFeel.cpp:855:27^M
[task 2017-10-02T07:26:41.522Z] 07:26:41     INFO - GECKO(1344) |     #5 0x7f6d38e0e14c in nsWebBrowser::Create() /builds/worker/work\
space/build/src/toolkit/components/browser/nsWebBrowser.cpp:1210:3^M

Comment 2

[Arthur Edelstein [:arthur]]

The offending values are being produced by the GTK function gtk_style_context_get_state(style);

According to the latest gtk source
https://github.com/GNOME/gtk/blob/master/gtk/gtkenums.h

  GTK_STATE_FLAG_DIR_LTR = 1 << 7,   //  128
  GTK_STATE_FLAG_CHECKED = 1 << 11,  // 2048

Maybe that means we are somehow including an old version of gtkenums.h that doesn't have these values?

Karl, do you have any insight? Thanks in advance.

Comment 3

[Karl Tomlinson (:karlt)]

Builds are typically against older versions, CentOS 6 gtk+ 3.4 IIRC, so that they run against newer versions.
It won't work the other way.

Perhaps your analysis can be run on a build on the same system?

These are really C enums.  The equivalence of C++ enums with C is dependent on implementation, but all implementations provide equivalence so that C++ can be used with C.

What is the warning protecting against?

Comment 4

[Karl Tomlinson (:karlt)]

In C++, although the underlying type is implementation defined, the conversion
from int to enum is undefined if the value is not within the range of
enumeration values.  The range of enumeration values depends only on the
enumerators, not on the underlying type.

In C, the compatible type is still implementation defined, but there is no
similar concept of a different range of enumeration values, and so no similar
*undefined* behavior.

C++ requires "linkage" to C functions.  Perhaps that means that behavior is
implementation defined when C code loads a value outside the range into
storage declared in C++?

The fact that GTK+ has added enumerators to increase the minimum precision of
the compatible type would seem to imply that it is assuming that compilers
were not using char or 8-bit integer compatible types.  GtkStateFlags was
always passed by value in the GTK+ API, but code compiled against GTK
versions with 8-bit enumerators are meant to successfully store > 8-bit values
when receiving GtkStateFlags from newer GTK versions so that they can be
passed back to GTK.


N4296

5.2.9
Static cast

10 A value of integral or enumeration type can be explicitly converted to a
complete enumeration type. The value is unchanged if the original value is
within the range of the enumeration values (7.2). Otherwise, the behavior is
undefined.

7.2 Enumeration declarations

7 For an enumeration whose underlying type is not fixed, the underlying type
is an integral type that can represent all the enumerator values defined in
the enumeration. If no integral type can represent all the enumerator values,
the enumeration is ill-formed. It is implementation-defined which integral
type is used as the underlying type except that the underlying type shall not
be larger than int unless the value of an enumerator cannot fit in an int or
unsigned int

8 For an enumeration whose underlying type is fixed, the values of the
enumeration are the values of the underlying type. Otherwise, for an
enumeration where e min is the smallest enumerator and e max is the largest,
the values of the enumeration are the values in the range b min to b max ,
defined as follows: Let K be 1 for a two’s complement representation and 0 for
a one’s complement or sign-magnitude representation.  b max is the smallest
value greater than or equal to max(|e min | − K, |e max |) and equal to 2 M −
1, where M is a non-negative integer. b min is zero if e min is non-negative
and −(b max + K) otherwise. The size of the smallest bit-field large enough to
hold all the values of the enumeration type is max(M, 1) if b min is zero and
M + 1 otherwise. It is possible to define an enumeration that has values not
defined by any of its enumerators. If the enumerator-list is empty, the values
of the enumeration are as if the enumeration had a single enumerator with
value 0. 96

96) This set of values is used to define promotion and conversion semantics
for the enumeration type. It does not preclude an expression of enumeration
type from having a value that falls outside this range.

7.5 Linkage specifications

3 Every implementation shall provide for linkage to functions written in the C
programming language, "C", and linkage to C ++ functions, "C++".


N1570 (C11)

6.2.5 Types

17 The type char, the signed and unsigned integer types, and the enumerated
types are collectively called integer types. The integer and real floating
types are collectively called real types.

6.3 Conversions

2 Conversion of an operand value to a compatible type causes no change to the
value or the representation.

6.4.4.3 Enumeration constants

2 An identifier declared as an enumeration constant has type int.

6.7.2.2 Enumeration specifiers

4 Each enumerated type shall be compatible with char, a signed integer type,
or an unsigned integer type. The choice of type is implementation-defined,
128) but shall be capable of representing the values of all the members of the
enumeration.

Comment 5

[Karl Tomlinson (:karlt)]

That still leaves undefined behavior where C++ code is converting to
GtkStateFlags.

Some options that would avoid performing the conversion in C++:

1) Use a C function to perform the conversion from integer to enumerated type.

2) Use C functions receiving integer types to call each GTK function receiving
   enumerated types.

3) Provide a different declaration for the GTK functions so that integral
   types are passed instead of enumeration types, and depend on the calling
   convention to sort it out.

Comment 6

[Karl Tomlinson (:karlt)]

The warnings from -fsanitize=enum are actually generated on load (not store)
of out-of-range values.  It's not clear to me that loading out-of-ranges
values is actually undefined behavior, when the values may derive from C code.

However, the best fix I think would be to modify
widget/gtk/compat-gtk3/gtk/gtkenums.h so as to temporarily #define
GtkStateFlags during #include_next <gtk/gtkenums.h> and to declare
GtkStateFlags with all the most recent enumerators.  That would provide
that all values are within range and also ensure that the underlying type
is of sufficient precision.