From 5c0f914f3859ae3920f7be6a618c17a47433bd85 Mon Sep 17 00:00:00 2001 From: Frank Benkstein Date: Thu, 18 Oct 2018 19:39:08 +0200 Subject: [PATCH] forbid enum values that are out of range (#4977) * 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 --- docs/source/Schemas.md | 23 ++++++++++++++++++++--- src/idl_parser.cpp | 41 +++++++++++++++++++++++++++++++++-------- tests/test.cpp | 26 +++++++++++++++++++++++++- 3 files changed, 78 insertions(+), 12 deletions(-) diff --git a/docs/source/Schemas.md b/docs/source/Schemas.md index 9647f7c..42551c6 100644 --- a/docs/source/Schemas.md +++ b/docs/source/Schemas.md @@ -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. @@ -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 diff --git a/src/idl_parser.cpp b/src/idl_parser.cpp index f0908ff..0eec353 100644 --- a/src/idl_parser.cpp +++ b/src/idl_parser.cpp @@ -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; @@ -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; @@ -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::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( \ + flatbuffers::numeric_limits::lowest()); \ + int64_t max_value = static_cast( \ + flatbuffers::numeric_limits::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. diff --git a/tests/test.cpp b/tests/test.cpp index 5bbac56..49f414c 100644 --- a/tests/test.cpp +++ b/tests/test.cpp @@ -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"); @@ -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 T TestValue(const char *json, const char *type_name) { @@ -1350,6 +1350,29 @@ void EnumNamesTest() { TEST_EQ_STR("", EnumNameColor(static_cast(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"); @@ -2375,6 +2398,7 @@ int FlatBufferTests() { ValueTest(); EnumStringsTest(); EnumNamesTest(); + EnumOutOfRangeTest(); IntegerOutOfRangeTest(); IntegerBoundaryTest(); UnicodeTest(); -- 2.7.4