Skip to content

P
protobuf-c
Commit 7634c0e6 authored by Ilya Lipnitskiy's avatar Ilya Lipnitskiy
Browse files
Options

protobuf-c.c: Pack negative enum values correctly 

Fix a few casts where ints were cast to uints unnecessarily

Fixes #199. Previously, enums were treated as uint32's, but they need to
be treated as int32's instead.

t: Add a few test cases with negative enum values
parent 8debedbc
Show whitespace changes
Inline Side-by-side
Showing with 34 additions and 21 deletions
protobuf-c/protobuf-c.c
View file @ 7634c0e6
...@@ -417,8 +417,9 @@ required_field_get_packed_size(const ProtobufCFieldDescriptor *field, ...@@ -417,8 +417,9 @@ required_field_get_packed_size(const ProtobufCFieldDescriptor *field,
switch (field->type) { switch (field->type) {
case PROTOBUF_C_TYPE_SINT32: case PROTOBUF_C_TYPE_SINT32:
return rv + sint32_size(*(const int32_t *) member); return rv + sint32_size(*(const int32_t *) member);
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
return rv + int32_size(*(const uint32_t *) member); return rv + int32_size(*(const int32_t *) member);
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
return rv + uint32_size(*(const uint32_t *) member); return rv + uint32_size(*(const uint32_t *) member);
case PROTOBUF_C_TYPE_SINT64: case PROTOBUF_C_TYPE_SINT64:
...@@ -438,9 +439,6 @@ required_field_get_packed_size(const ProtobufCFieldDescriptor *field, ...@@ -438,9 +439,6 @@ required_field_get_packed_size(const ProtobufCFieldDescriptor *field,
return rv + 4; return rv + 4;
case PROTOBUF_C_TYPE_DOUBLE: case PROTOBUF_C_TYPE_DOUBLE:
return rv + 8; return rv + 8;
case PROTOBUF_C_TYPE_ENUM:
/* \todo Is this correct for negative-valued enums? */
return rv + uint32_size(*(const uint32_t *) member);
case PROTOBUF_C_TYPE_STRING: { case PROTOBUF_C_TYPE_STRING: {
const char *str = *(char * const *) member; const char *str = *(char * const *) member;
size_t len = str ? strlen(str) : 0; size_t len = str ? strlen(str) : 0;
...@@ -559,12 +557,12 @@ repeated_field_get_packed_size(const ProtobufCFieldDescriptor *field, ...@@ -559,12 +557,12 @@ repeated_field_get_packed_size(const ProtobufCFieldDescriptor *field,
for (i = 0; i < count; i++) for (i = 0; i < count; i++)
rv += sint32_size(((int32_t *) array)[i]); rv += sint32_size(((int32_t *) array)[i]);
break; break;
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
for (i = 0; i < count; i++) for (i = 0; i < count; i++)
rv += int32_size(((uint32_t *) array)[i]); rv += int32_size(((int32_t *) array)[i]);
break; break;
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
case PROTOBUF_C_TYPE_ENUM:
for (i = 0; i < count; i++) for (i = 0; i < count; i++)
rv += uint32_size(((uint32_t *) array)[i]); rv += uint32_size(((uint32_t *) array)[i]);
break; break;
...@@ -1013,11 +1011,11 @@ required_field_pack(const ProtobufCFieldDescriptor *field, ...@@ -1013,11 +1011,11 @@ required_field_pack(const ProtobufCFieldDescriptor *field,
case PROTOBUF_C_TYPE_SINT32: case PROTOBUF_C_TYPE_SINT32:
out[0] |= PROTOBUF_C_WIRE_TYPE_VARINT; out[0] |= PROTOBUF_C_WIRE_TYPE_VARINT;
return rv + sint32_pack(*(const int32_t *) member, out + rv); return rv + sint32_pack(*(const int32_t *) member, out + rv);
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
out[0] |= PROTOBUF_C_WIRE_TYPE_VARINT; out[0] |= PROTOBUF_C_WIRE_TYPE_VARINT;
return rv + int32_pack(*(const uint32_t *) member, out + rv); return rv + int32_pack(*(const int32_t *) member, out + rv);
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
case PROTOBUF_C_TYPE_ENUM:
out[0] |= PROTOBUF_C_WIRE_TYPE_VARINT; out[0] |= PROTOBUF_C_WIRE_TYPE_VARINT;
return rv + uint32_pack(*(const uint32_t *) member, out + rv); return rv + uint32_pack(*(const uint32_t *) member, out + rv);
case PROTOBUF_C_TYPE_SINT64: case PROTOBUF_C_TYPE_SINT64:
...@@ -1288,6 +1286,7 @@ repeated_field_pack(const ProtobufCFieldDescriptor *field, ...@@ -1288,6 +1286,7 @@ repeated_field_pack(const ProtobufCFieldDescriptor *field,
copy_to_little_endian_64(payload_at, array, count); copy_to_little_endian_64(payload_at, array, count);
payload_at += count * 8; payload_at += count * 8;
break; break;
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: { case PROTOBUF_C_TYPE_INT32: {
const int32_t *arr = (const int32_t *) array; const int32_t *arr = (const int32_t *) array;
for (i = 0; i < count; i++) for (i = 0; i < count; i++)
...@@ -1306,7 +1305,6 @@ repeated_field_pack(const ProtobufCFieldDescriptor *field, ...@@ -1306,7 +1305,6 @@ repeated_field_pack(const ProtobufCFieldDescriptor *field,
payload_at += sint64_pack(arr[i], payload_at); payload_at += sint64_pack(arr[i], payload_at);
break; break;
} }
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_UINT32: { case PROTOBUF_C_TYPE_UINT32: {
const uint32_t *arr = (const uint32_t *) array; const uint32_t *arr = (const uint32_t *) array;
for (i = 0; i < count; i++) for (i = 0; i < count; i++)
...@@ -1441,13 +1439,13 @@ required_field_pack_to_buffer(const ProtobufCFieldDescriptor *field, ...@@ -1441,13 +1439,13 @@ required_field_pack_to_buffer(const ProtobufCFieldDescriptor *field,
rv += sint32_pack(*(const int32_t *) member, scratch + rv); rv += sint32_pack(*(const int32_t *) member, scratch + rv);
buffer->append(buffer, rv, scratch); buffer->append(buffer, rv, scratch);
break; break;
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
scratch[0] |= PROTOBUF_C_WIRE_TYPE_VARINT; scratch[0] |= PROTOBUF_C_WIRE_TYPE_VARINT;
rv += int32_pack(*(const uint32_t *) member, scratch + rv); rv += int32_pack(*(const int32_t *) member, scratch + rv);
buffer->append(buffer, rv, scratch); buffer->append(buffer, rv, scratch);
break; break;
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
case PROTOBUF_C_TYPE_ENUM:
scratch[0] |= PROTOBUF_C_WIRE_TYPE_VARINT; scratch[0] |= PROTOBUF_C_WIRE_TYPE_VARINT;
rv += uint32_pack(*(const uint32_t *) member, scratch + rv); rv += uint32_pack(*(const uint32_t *) member, scratch + rv);
buffer->append(buffer, rv, scratch); buffer->append(buffer, rv, scratch);
...@@ -1622,6 +1620,7 @@ get_packed_payload_length(const ProtobufCFieldDescriptor *field, ...@@ -1622,6 +1620,7 @@ get_packed_payload_length(const ProtobufCFieldDescriptor *field,
case PROTOBUF_C_TYPE_FIXED64: case PROTOBUF_C_TYPE_FIXED64:
case PROTOBUF_C_TYPE_DOUBLE: case PROTOBUF_C_TYPE_DOUBLE:
return count * 8; return count * 8;
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: { case PROTOBUF_C_TYPE_INT32: {
const int32_t *arr = (const int32_t *) array; const int32_t *arr = (const int32_t *) array;
for (i = 0; i < count; i++) for (i = 0; i < count; i++)
...@@ -1634,7 +1633,6 @@ get_packed_payload_length(const ProtobufCFieldDescriptor *field, ...@@ -1634,7 +1633,6 @@ get_packed_payload_length(const ProtobufCFieldDescriptor *field,
rv += sint32_size(arr[i]); rv += sint32_size(arr[i]);
break; break;
} }
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_UINT32: { case PROTOBUF_C_TYPE_UINT32: {
const uint32_t *arr = (const uint32_t *) array; const uint32_t *arr = (const uint32_t *) array;
for (i = 0; i < count; i++) for (i = 0; i < count; i++)
...@@ -1714,6 +1712,7 @@ pack_buffer_packed_payload(const ProtobufCFieldDescriptor *field, ...@@ -1714,6 +1712,7 @@ pack_buffer_packed_payload(const ProtobufCFieldDescriptor *field,
} }
break; break;
#endif #endif
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
for (i = 0; i < count; i++) { for (i = 0; i < count; i++) {
unsigned len = int32_pack(((int32_t *) array)[i], scratch); unsigned len = int32_pack(((int32_t *) array)[i], scratch);
...@@ -1728,7 +1727,6 @@ pack_buffer_packed_payload(const ProtobufCFieldDescriptor *field, ...@@ -1728,7 +1727,6 @@ pack_buffer_packed_payload(const ProtobufCFieldDescriptor *field,
rv += len; rv += len;
} }
break; break;
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
for (i = 0; i < count; i++) { for (i = 0; i < count; i++) {
unsigned len = uint32_pack(((uint32_t *) array)[i], scratch); unsigned len = uint32_pack(((uint32_t *) array)[i], scratch);
...@@ -2216,9 +2214,9 @@ count_packed_elements(ProtobufCType type, ...@@ -2216,9 +2214,9 @@ count_packed_elements(ProtobufCType type,
} }
*count_out = len / 8; *count_out = len / 8;
return TRUE; return TRUE;
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
case PROTOBUF_C_TYPE_SINT32: case PROTOBUF_C_TYPE_SINT32:
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
case PROTOBUF_C_TYPE_INT64: case PROTOBUF_C_TYPE_INT64:
case PROTOBUF_C_TYPE_SINT64: case PROTOBUF_C_TYPE_SINT64:
...@@ -2348,10 +2346,11 @@ parse_required_member(ScannedMember *scanned_member, ...@@ -2348,10 +2346,11 @@ parse_required_member(ScannedMember *scanned_member,
ProtobufCWireType wire_type = scanned_member->wire_type; ProtobufCWireType wire_type = scanned_member->wire_type;
switch (scanned_member->field->type) { switch (scanned_member->field->type) {
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
if (wire_type != PROTOBUF_C_WIRE_TYPE_VARINT) if (wire_type != PROTOBUF_C_WIRE_TYPE_VARINT)
return FALSE; return FALSE;
*(uint32_t *) member = parse_int32(len, data); *(int32_t *) member = parse_int32(len, data);
return TRUE; return TRUE;
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
if (wire_type != PROTOBUF_C_WIRE_TYPE_VARINT) if (wire_type != PROTOBUF_C_WIRE_TYPE_VARINT)
...@@ -2391,11 +2390,6 @@ parse_required_member(ScannedMember *scanned_member, ...@@ -2391,11 +2390,6 @@ parse_required_member(ScannedMember *scanned_member,
case PROTOBUF_C_TYPE_BOOL: case PROTOBUF_C_TYPE_BOOL:
*(protobuf_c_boolean *) member = parse_boolean(len, data); *(protobuf_c_boolean *) member = parse_boolean(len, data);
return TRUE; return TRUE;
case PROTOBUF_C_TYPE_ENUM:
if (wire_type != PROTOBUF_C_WIRE_TYPE_VARINT)
return FALSE;
*(uint32_t *) member = parse_uint32(len, data);
return TRUE;
case PROTOBUF_C_TYPE_STRING: { case PROTOBUF_C_TYPE_STRING: {
char **pstr = member; char **pstr = member;
unsigned pref_len = scanned_member->length_prefix_len; unsigned pref_len = scanned_member->length_prefix_len;
...@@ -2623,6 +2617,7 @@ parse_packed_repeated_member(ScannedMember *scanned_member, ...@@ -2623,6 +2617,7 @@ parse_packed_repeated_member(ScannedMember *scanned_member,
} }
break; break;
#endif #endif
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_INT32: case PROTOBUF_C_TYPE_INT32:
while (rem > 0) { while (rem > 0) {
unsigned s = scan_varint(rem, at); unsigned s = scan_varint(rem, at);
...@@ -2647,7 +2642,6 @@ parse_packed_repeated_member(ScannedMember *scanned_member, ...@@ -2647,7 +2642,6 @@ parse_packed_repeated_member(ScannedMember *scanned_member,
rem -= s; rem -= s;
} }
break; break;
case PROTOBUF_C_TYPE_ENUM:
case PROTOBUF_C_TYPE_UINT32: case PROTOBUF_C_TYPE_UINT32:
while (rem > 0) { while (rem > 0) {
unsigned s = scan_varint(rem, at); unsigned s = scan_varint(rem, at);
......
t/generated-code2/cxx-generate-packed-data.cc
View file @ 7634c0e6
...@@ -69,6 +69,8 @@ static void ...@@ -69,6 +69,8 @@ static void
dump_test_enum_small (void) dump_test_enum_small (void)
{ {
TestMessRequiredEnumSmall es; TestMessRequiredEnumSmall es;
es.set_test(NEG_VALUE);
dump_message_bytes(&es, "test_enum_small_NEG_VALUE");
es.set_test(VALUE); es.set_test(VALUE);
dump_message_bytes(&es, "test_enum_small_VALUE"); dump_message_bytes(&es, "test_enum_small_VALUE");
es.set_test(OTHER_VALUE); es.set_test(OTHER_VALUE);
...@@ -78,6 +80,8 @@ static void ...@@ -78,6 +80,8 @@ static void
dump_test_enum_big (void) dump_test_enum_big (void)
{ {
TestMessRequiredEnum eb; TestMessRequiredEnum eb;
eb.set_test(VALUENEG123456); dump_message_bytes(&eb, "test_enum_big_VALUENEG123456");
eb.set_test(VALUENEG1); dump_message_bytes(&eb, "test_enum_big_VALUENEG1");
eb.set_test(VALUE0); dump_message_bytes(&eb, "test_enum_big_VALUE0"); eb.set_test(VALUE0); dump_message_bytes(&eb, "test_enum_big_VALUE0");
eb.set_test(VALUE127); dump_message_bytes(&eb, "test_enum_big_VALUE127"); eb.set_test(VALUE127); dump_message_bytes(&eb, "test_enum_big_VALUE127");
eb.set_test(VALUE128); dump_message_bytes(&eb, "test_enum_big_VALUE128"); eb.set_test(VALUE128); dump_message_bytes(&eb, "test_enum_big_VALUE128");
...@@ -298,6 +302,8 @@ static void dump_test_required_enum_small (void) ...@@ -298,6 +302,8 @@ static void dump_test_required_enum_small (void)
static void dump_test_required_enum (void) static void dump_test_required_enum (void)
{ {
TestMessRequiredEnum mess; TestMessRequiredEnum mess;
mess.set_test (VALUENEG1);
dump_message_bytes (&mess, "test_required_enum_neg1");
mess.set_test (VALUE0); mess.set_test (VALUE0);
dump_message_bytes (&mess, "test_required_enum_0"); dump_message_bytes (&mess, "test_required_enum_0");
mess.set_test (VALUE1); mess.set_test (VALUE1);
...@@ -515,6 +521,8 @@ static void dump_test_optional_bool (void) ...@@ -515,6 +521,8 @@ static void dump_test_optional_bool (void)
static void dump_test_optional_enum_small (void) static void dump_test_optional_enum_small (void)
{ {
TestMessOptional opt; TestMessOptional opt;
opt.set_test_enum_small (NEG_VALUE);
dump_message_bytes (&opt, "test_optional_enum_small_neg1");
opt.set_test_enum_small (VALUE); opt.set_test_enum_small (VALUE);
dump_message_bytes (&opt, "test_optional_enum_small_0"); dump_message_bytes (&opt, "test_optional_enum_small_0");
opt.set_test_enum_small (OTHER_VALUE); opt.set_test_enum_small (OTHER_VALUE);
...@@ -527,6 +535,8 @@ static void dump_test_optional_enum (void) ...@@ -527,6 +535,8 @@ static void dump_test_optional_enum (void)
//echo ' opt.set_test_enum (VALUE'$a'); //echo ' opt.set_test_enum (VALUE'$a');
//dump_message_bytes (&opt, "test_optional_enum_'$a'");' //dump_message_bytes (&opt, "test_optional_enum_'$a'");'
//done //done
opt.set_test_enum (VALUENEG1);
dump_message_bytes (&opt, "test_optional_enum_neg1");
opt.set_test_enum (VALUE0); opt.set_test_enum (VALUE0);
dump_message_bytes (&opt, "test_optional_enum_0"); dump_message_bytes (&opt, "test_optional_enum_0");
opt.set_test_enum (VALUE1); opt.set_test_enum (VALUE1);
......
t/generated-code2/test-generated-code2.c
View file @ 7634c0e6
...@@ -132,6 +132,7 @@ static void test_enum_small (void) ...@@ -132,6 +132,7 @@ static void test_enum_small (void)
assert (sizeof (Foo__TestEnumSmall) == 4); assert (sizeof (Foo__TestEnumSmall) == 4);
DO_TEST(NEG_VALUE);
DO_TEST(VALUE); DO_TEST(VALUE);
DO_TEST(OTHER_VALUE); DO_TEST(OTHER_VALUE);
...@@ -159,6 +160,8 @@ static void test_enum_big (void) ...@@ -159,6 +160,8 @@ static void test_enum_big (void)
free (data); \ free (data); \
}while(0) }while(0)
DO_ONE_TEST(VALUENEG123456, -123456, 10);
DO_ONE_TEST(VALUENEG1, -1, 10);
DO_ONE_TEST(VALUE0, 0, 1); DO_ONE_TEST(VALUE0, 0, 1);
DO_ONE_TEST(VALUE127, 127, 1); DO_ONE_TEST(VALUE127, 127, 1);
DO_ONE_TEST(VALUE128, 128, 2); DO_ONE_TEST(VALUE128, 128, 2);
...@@ -375,6 +378,7 @@ static void test_required_TestEnum (void) ...@@ -375,6 +378,7 @@ static void test_required_TestEnum (void)
#define DO_TEST(value, example_packed_data) \ #define DO_TEST(value, example_packed_data) \
DO_TEST_REQUIRED(Enum, ENUM, enum, value, example_packed_data, NUMERIC_EQUALS) DO_TEST_REQUIRED(Enum, ENUM, enum, value, example_packed_data, NUMERIC_EQUALS)
DO_TEST (FOO__TEST_ENUM__VALUENEG1, test_required_enum_neg1);
DO_TEST (FOO__TEST_ENUM__VALUE0, test_required_enum_0); DO_TEST (FOO__TEST_ENUM__VALUE0, test_required_enum_0);
DO_TEST (FOO__TEST_ENUM__VALUE1, test_required_enum_1); DO_TEST (FOO__TEST_ENUM__VALUE1, test_required_enum_1);
DO_TEST (FOO__TEST_ENUM__VALUE127, test_required_enum_127); DO_TEST (FOO__TEST_ENUM__VALUE127, test_required_enum_127);
...@@ -630,6 +634,7 @@ static void test_optional_TestEnum (void) ...@@ -630,6 +634,7 @@ static void test_optional_TestEnum (void)
#define DO_TEST(value, example_packed_data) \ #define DO_TEST(value, example_packed_data) \
DO_TEST_OPTIONAL(test_enum, value, example_packed_data, NUMERIC_EQUALS) DO_TEST_OPTIONAL(test_enum, value, example_packed_data, NUMERIC_EQUALS)
DO_TEST (FOO__TEST_ENUM__VALUENEG1, test_optional_enum_neg1);
DO_TEST (FOO__TEST_ENUM__VALUE0, test_optional_enum_0); DO_TEST (FOO__TEST_ENUM__VALUE0, test_optional_enum_0);
DO_TEST (FOO__TEST_ENUM__VALUE1, test_optional_enum_1); DO_TEST (FOO__TEST_ENUM__VALUE1, test_optional_enum_1);
DO_TEST (FOO__TEST_ENUM__VALUE127, test_optional_enum_127); DO_TEST (FOO__TEST_ENUM__VALUE127, test_optional_enum_127);
...@@ -911,6 +916,7 @@ static void test_oneof_TestEnum (void) ...@@ -911,6 +916,7 @@ static void test_oneof_TestEnum (void)
#define DO_TEST(value, example_packed_data) \ #define DO_TEST(value, example_packed_data) \
DO_TEST_ONEOF(test_enum, TEST_ENUM, value, example_packed_data, GENERIC_ASSIGN, NUMERIC_EQUALS) DO_TEST_ONEOF(test_enum, TEST_ENUM, value, example_packed_data, GENERIC_ASSIGN, NUMERIC_EQUALS)
DO_TEST (FOO__TEST_ENUM__VALUENEG1, test_optional_enum_neg1);
DO_TEST (FOO__TEST_ENUM__VALUE0, test_optional_enum_0); DO_TEST (FOO__TEST_ENUM__VALUE0, test_optional_enum_0);
DO_TEST (FOO__TEST_ENUM__VALUE1, test_optional_enum_1); DO_TEST (FOO__TEST_ENUM__VALUE1, test_optional_enum_1);
DO_TEST (FOO__TEST_ENUM__VALUE127, test_optional_enum_127); DO_TEST (FOO__TEST_ENUM__VALUE127, test_optional_enum_127);
......
t/test-full.proto
View file @ 7634c0e6
...@@ -17,6 +17,7 @@ message SubMess { ...@@ -17,6 +17,7 @@ message SubMess {
}; };
enum TestEnumSmall { enum TestEnumSmall {
NEG_VALUE = -1;
VALUE = 0; VALUE = 0;
OTHER_VALUE = 1; OTHER_VALUE = 1;
} }
...@@ -25,6 +26,8 @@ enum TestEnumSmall { ...@@ -25,6 +26,8 @@ enum TestEnumSmall {
// boundaries of when an enum requires a certain number of bytes. // boundaries of when an enum requires a certain number of bytes.
// e.g. 16383 requires 3 bytes; 16384 requires 4. // e.g. 16383 requires 3 bytes; 16384 requires 4.
enum TestEnum { enum TestEnum {
VALUENEG123456 = -123456;
VALUENEG1 = -1;
VALUE0 = 0; VALUE0 = 0;
VALUE1 = 1; VALUE1 = 1;
VALUE127 = 127; VALUE127 = 127;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment