Skip to content

Commit

Permalink
forbid enum values that are out of range (google#4977)
Browse files Browse the repository at this point in the history 
* forbid enum values that are out of range

Enum values that are out of range can lead to generated C++ code that does
not compile.  Also forbid boolean enums.

* update enum and union documentation slightly
  • Loading branch information
@fbenkstein @aardappel
fbenkstein authored and aardappel committed Oct 18, 2018
1 parent 802639e commit 5c0f914
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 12 deletions.
23 changes: 20 additions & 3 deletions docs/source/Schemas.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ is `0`. As you can see in the enum declaration, you specify the underlying
integral type of the enum with `:` (in this case `byte`), which then determines
the type of any fields declared with this enum type.

Only integer types are allowed, i.e. `byte`, `ubyte`, `short` `ushort`, `int`,
`uint`, `long` and `ulong`.

Typically, enum values should only ever be added, never removed (there is no
deprecation for enums). This requires code to handle forwards compatibility
itself, by handling unknown enum values.
Expand All @@ -150,9 +153,23 @@ itself, by handling unknown enum values.
Unions share a lot of properties with enums, but instead of new names
for constants, you use names of tables. You can then declare
a union field, which can hold a reference to any of those types, and
additionally a hidden field with the suffix `_type` is generated that
holds the corresponding enum value, allowing you to know which type to
cast to at runtime.
additionally a field with the suffix `_type` is generated that holds
the corresponding enum value, allowing you to know which type to cast
to at runtime.

It's possible to give an alias name to a type union. This way a type can even be
used to mean different things depending on the name used:

table PointPosition { x:uint; y:uint; }
table MarkerPosition {}
union Position {
Start:MarkerPosition,
Point:PointPosition,
Finish:MarkerPosition
}

Unions contain a special `NONE` marker to denote that no value is stored so that
name cannot be used as an alias.

Unions are a good way to be able to send multiple message types as a FlatBuffer.
Note that because a union field is really two fields, it must always be
Expand Down
41 changes: 33 additions & 8 deletions src/idl_parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1594,7 +1594,8 @@ CheckedError Parser::ParseEnum(bool is_union, EnumDef **dest) {
}
// Specify the integer type underlying this enum.
ECHECK(ParseType(enum_def->underlying_type));
if (!IsInteger(enum_def->underlying_type.base_type))
if (!IsInteger(enum_def->underlying_type.base_type) ||
IsBool(enum_def->underlying_type.base_type))
return Error("underlying enum type must be integral");
// Make this type refer back to the enum it was derived from.
enum_def->underlying_type.enum_def = enum_def;
Expand All @@ -1620,10 +1621,8 @@ CheckedError Parser::ParseEnum(bool is_union, EnumDef **dest) {
}
}
auto prevsize = enum_def->vals.vec.size();
auto value = !enum_def->vals.vec.empty()
? enum_def->vals.vec.back()->value + 1
: 0;
auto &ev = *new EnumVal(value_name, value);
auto prevvalue = prevsize > 0 ? enum_def->vals.vec.back()->value : 0;
auto &ev = *new EnumVal(value_name, 0);
if (enum_def->vals.Add(value_name, &ev))
return Error("enum value already exists: " + value_name);
ev.doc_comment = value_comment;
Expand All @@ -1646,11 +1645,37 @@ CheckedError Parser::ParseEnum(bool is_union, EnumDef **dest) {
if (!opts.proto_mode && prevsize &&
enum_def->vals.vec[prevsize - 1]->value >= ev.value)
return Error("enum values must be specified in ascending order");
} else if (prevsize == 0) {
// already set to zero
} else if (prevvalue != flatbuffers::numeric_limits<int64_t>::max()) {
ev.value = prevvalue + 1;
} else {
return Error("enum value overflows");
}
if (is_union) {
if (ev.value < 0 || ev.value >= 256)
return Error("union enum value must fit in a ubyte");

// Check that value fits into the underlying type.
switch (enum_def->underlying_type.base_type) {
// clang-format off
#define FLATBUFFERS_TD(ENUM, IDLTYPE, CTYPE, JTYPE, GTYPE, NTYPE, \
PTYPE, RTYPE) \
case BASE_TYPE_##ENUM: { \
int64_t min_value = static_cast<int64_t>( \
flatbuffers::numeric_limits<CTYPE>::lowest()); \
int64_t max_value = static_cast<int64_t>( \
flatbuffers::numeric_limits<CTYPE>::max()); \
if (ev.value < min_value || ev.value > max_value) { \
return Error( \
"enum value does not fit [" + NumToString(min_value) + \
"; " + NumToString(max_value) + "]"); \
} \
break; \
}
FLATBUFFERS_GEN_TYPES_SCALAR(FLATBUFFERS_TD);
#undef FLATBUFFERS_TD
default: break;
// clang-format on
}

if (opts.proto_mode && Is('[')) {
NEXT();
// ignore attributes on enums.
Expand Down
26 changes: 25 additions & 1 deletion tests/test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1227,7 +1227,6 @@ void ErrorTest() {
TestError("enum X:float {}", "underlying");
TestError("enum X:byte { Y, Y }", "value already");
TestError("enum X:byte { Y=2, Z=1 }", "ascending");
TestError("union X { Y = 256 }", "must fit");
TestError("enum X:byte (bit_flags) { Y=8 }", "bit flag out");
TestError("table X { Y:int; } table X {", "datatype already");
TestError("struct X (force_align: 7) { Y:int; }", "force_align");
Expand All @@ -1244,6 +1243,7 @@ void ErrorTest() {
// float to integer conversion is forbidden
TestError("table X { Y:int; } root_type X; { Y:1.0 }", "float");
TestError("table X { Y:bool; } root_type X; { Y:1.0 }", "float");
TestError("enum X:bool { Y = true }", "must be integral");
}

template<typename T> T TestValue(const char *json, const char *type_name) {
Expand Down Expand Up @@ -1350,6 +1350,29 @@ void EnumNamesTest() {
TEST_EQ_STR("", EnumNameColor(static_cast<Color>(1000)));
}

void EnumOutOfRangeTest() {
TestError("enum X:byte { Y = 128 }", "enum value does not fit");
TestError("enum X:byte { Y = -129 }", "enum value does not fit");
TestError("enum X:byte { Y = 127, Z }", "enum value does not fit");
TestError("enum X:ubyte { Y = -1 }", "enum value does not fit");
TestError("enum X:ubyte { Y = 256 }", "enum value does not fit");
// Unions begin with an implicit "NONE = 0".
TestError("table Y{} union X { Y = -1 }",
"enum values must be specified in ascending order");
TestError("table Y{} union X { Y = 256 }", "enum value does not fit");
TestError("table Y{} union X { Y = 255, Z:Y }", "enum value does not fit");
TestError("enum X:int { Y = -2147483649 }", "enum value does not fit");
TestError("enum X:int { Y = 2147483648 }", "enum value does not fit");
TestError("enum X:uint { Y = -1 }", "enum value does not fit");
TestError("enum X:uint { Y = 4294967297 }", "enum value does not fit");
TestError("enum X:long { Y = 9223372036854775808 }", "constant does not fit");
TestError("enum X:long { Y = 9223372036854775807, Z }", "enum value overflows");
TestError("enum X:ulong { Y = -1 }", "enum value does not fit");
// TODO: these are perfectly valid constants that shouldn't fail
TestError("enum X:ulong { Y = 13835058055282163712 }", "constant does not fit");
TestError("enum X:ulong { Y = 18446744073709551615 }", "constant does not fit");
}

void IntegerOutOfRangeTest() {
TestError("table T { F:byte; } root_type T; { F:128 }",
"constant does not fit");
Expand Down Expand Up @@ -2375,6 +2398,7 @@ int FlatBufferTests() {
ValueTest();
EnumStringsTest();
EnumNamesTest();
EnumOutOfRangeTest();
IntegerOutOfRangeTest();
IntegerBoundaryTest();
UnicodeTest();
Expand Down

0 comments on commit 5c0f914

Please sign in to comment.