From: Simon McVittie Date: Thu, 6 Dec 2018 13:49:56 +0000 (+0000) Subject: Don't cast user-supplied pointers to DBusBasicValue * X-Git-Tag: dbus-1.13.10~41^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=26d5d97d5b33e4da30fde21676ee212fef1c5ea3;p=thirdparty%2Fdbus.git Don't cast user-supplied pointers to DBusBasicValue * If we cast a user-supplied pointer to DBusBasicValue *, that's like promising that the pointer is to an object at least as strictly aligned as a DBusBasicValue. In practice, it often isn't: for example, when we marshal a dbus_uint32_t, a pointer to it is only guaranteed to be 32-bit-aligned, whereas DBusBasicValue typically requires 64-bit alignment. Signed-off-by: Simon McVittie --- diff --git a/dbus/dbus-marshal-basic.c b/dbus/dbus-marshal-basic.c index 93daa09e3..ba9a935cd 100644 --- a/dbus/dbus-marshal-basic.c +++ b/dbus/dbus-marshal-basic.c @@ -37,11 +37,15 @@ #if defined(__GNUC__) && (__GNUC__ >= 4) # define _DBUS_ASSERT_ALIGNMENT(type, op, val) \ _DBUS_STATIC_ASSERT (__extension__ __alignof__ (type) op val) +# define _DBUS_ASSERT_CMP_ALIGNMENT(left, op, right) \ + _DBUS_STATIC_ASSERT (__extension__ __alignof__ (left) op __extension__ __alignof__ (right)) #else /* not gcc, so probably no alignof operator: just use a no-op statement * that's valid in the same contexts */ # define _DBUS_ASSERT_ALIGNMENT(type, op, val) \ _DBUS_STATIC_ASSERT (TRUE) +# define _DBUS_ASSERT_CMP_ALIGNMENT(left, op, right) \ + _DBUS_STATIC_ASSERT (TRUE) #endif /* True by definition, but just for completeness... */ @@ -52,21 +56,30 @@ _DBUS_STATIC_ASSERT (sizeof (dbus_int16_t) == 2); _DBUS_ASSERT_ALIGNMENT (dbus_int16_t, <=, 2); _DBUS_STATIC_ASSERT (sizeof (dbus_uint16_t) == 2); _DBUS_ASSERT_ALIGNMENT (dbus_uint16_t, <=, 2); +_DBUS_ASSERT_CMP_ALIGNMENT (dbus_uint16_t, ==, dbus_int16_t); _DBUS_STATIC_ASSERT (sizeof (dbus_int32_t) == 4); _DBUS_ASSERT_ALIGNMENT (dbus_int32_t, <=, 4); _DBUS_STATIC_ASSERT (sizeof (dbus_uint32_t) == 4); _DBUS_ASSERT_ALIGNMENT (dbus_uint32_t, <=, 4); +_DBUS_ASSERT_CMP_ALIGNMENT (dbus_uint32_t, ==, dbus_int32_t); _DBUS_STATIC_ASSERT (sizeof (dbus_bool_t) == 4); _DBUS_ASSERT_ALIGNMENT (dbus_bool_t, <=, 4); +_DBUS_ASSERT_CMP_ALIGNMENT (dbus_uint32_t, ==, dbus_bool_t); _DBUS_STATIC_ASSERT (sizeof (double) == 8); _DBUS_ASSERT_ALIGNMENT (double, <=, 8); +/* Doubles might sometimes be more strictly aligned than int64, but we + * assume they are no less strictly aligned. This means every (double *) + * has enough alignment to be treated as though it was a + * (dbus_uint64_t *). */ +_DBUS_ASSERT_CMP_ALIGNMENT (dbus_uint64_t, <=, double); _DBUS_STATIC_ASSERT (sizeof (dbus_int64_t) == 8); _DBUS_ASSERT_ALIGNMENT (dbus_int64_t, <=, 8); _DBUS_STATIC_ASSERT (sizeof (dbus_uint64_t) == 8); _DBUS_ASSERT_ALIGNMENT (dbus_uint64_t, <=, 8); +_DBUS_ASSERT_CMP_ALIGNMENT (dbus_uint64_t, ==, dbus_int64_t); _DBUS_STATIC_ASSERT (sizeof (DBusBasicValue) >= 8); /* The alignment of a DBusBasicValue might conceivably be > 8 because of the @@ -117,16 +130,16 @@ pack_4_octets (dbus_uint32_t value, } static void -pack_8_octets (DBusBasicValue value, +pack_8_octets (dbus_uint64_t value, int byte_order, unsigned char *data) { _dbus_assert (_DBUS_ALIGN_ADDRESS (data, 8) == data); if ((byte_order) == DBUS_LITTLE_ENDIAN) - *((dbus_uint64_t*)(data)) = DBUS_UINT64_TO_LE (value.u64); + *((dbus_uint64_t*)(data)) = DBUS_UINT64_TO_LE (value); else - *((dbus_uint64_t*)(data)) = DBUS_UINT64_TO_BE (value.u64); + *((dbus_uint64_t*)(data)) = DBUS_UINT64_TO_BE (value); } /** @@ -145,12 +158,12 @@ _dbus_pack_uint32 (dbus_uint32_t value, } static void -swap_8_octets (DBusBasicValue *value, +swap_8_octets (dbus_uint64_t *value, int byte_order) { if (byte_order != DBUS_COMPILER_BYTE_ORDER) { - value->u64 = DBUS_UINT64_SWAP_LE_BE (value->u64); + *value = DBUS_UINT64_SWAP_LE_BE (*value); } } @@ -231,7 +244,7 @@ set_4_octets (DBusString *str, static void set_8_octets (DBusString *str, int offset, - DBusBasicValue value, + dbus_uint64_t value, int byte_order) { char *data; @@ -380,14 +393,24 @@ _dbus_marshal_set_basic (DBusString *str, int *old_end_pos, int *new_end_pos) { - const DBusBasicValue *vp; - - vp = value; + /* Static assertions near the top of this file assert that signed and + * unsigned 16- and 32-bit quantities have the same alignment, and that + * doubles have alignment at least as strict as unsigned int64, so we + * don't have to distinguish further: every (double *) + * has strong enough alignment to be treated as though it was a + * (dbus_uint64_t *). Going via a (void *) means the compiler should + * know that pointers can alias each other. */ + const unsigned char *u8_p; + const dbus_uint16_t *u16_p; + const dbus_uint32_t *u32_p; + const dbus_uint64_t *u64_p; + const char * const *string_p; switch (type) { case DBUS_TYPE_BYTE: - _dbus_string_set_byte (str, pos, vp->byt); + u8_p = value; + _dbus_string_set_byte (str, pos, *u8_p); if (old_end_pos) *old_end_pos = pos + 1; if (new_end_pos) @@ -396,8 +419,9 @@ _dbus_marshal_set_basic (DBusString *str, break; case DBUS_TYPE_INT16: case DBUS_TYPE_UINT16: + u16_p = value; pos = _DBUS_ALIGN_VALUE (pos, 2); - set_2_octets (str, pos, vp->u16, byte_order); + set_2_octets (str, pos, *u16_p, byte_order); if (old_end_pos) *old_end_pos = pos + 2; if (new_end_pos) @@ -408,8 +432,9 @@ _dbus_marshal_set_basic (DBusString *str, case DBUS_TYPE_INT32: case DBUS_TYPE_UINT32: case DBUS_TYPE_UNIX_FD: + u32_p = value; pos = _DBUS_ALIGN_VALUE (pos, 4); - set_4_octets (str, pos, vp->u32, byte_order); + set_4_octets (str, pos, *u32_p, byte_order); if (old_end_pos) *old_end_pos = pos + 4; if (new_end_pos) @@ -419,8 +444,9 @@ _dbus_marshal_set_basic (DBusString *str, case DBUS_TYPE_INT64: case DBUS_TYPE_UINT64: case DBUS_TYPE_DOUBLE: + u64_p = value; pos = _DBUS_ALIGN_VALUE (pos, 8); - set_8_octets (str, pos, *vp, byte_order); + set_8_octets (str, pos, *u64_p, byte_order); if (old_end_pos) *old_end_pos = pos + 8; if (new_end_pos) @@ -429,14 +455,16 @@ _dbus_marshal_set_basic (DBusString *str, break; case DBUS_TYPE_STRING: case DBUS_TYPE_OBJECT_PATH: + string_p = value; pos = _DBUS_ALIGN_VALUE (pos, 4); - _dbus_assert (vp->str != NULL); - return set_string (str, pos, vp->str, byte_order, + _dbus_assert (*string_p != NULL); + return set_string (str, pos, *string_p, byte_order, old_end_pos, new_end_pos); break; case DBUS_TYPE_SIGNATURE: - _dbus_assert (vp->str != NULL); - return set_signature (str, pos, vp->str, byte_order, + string_p = value; + _dbus_assert (*string_p != NULL); + return set_signature (str, pos, *string_p, byte_order, old_end_pos, new_end_pos); break; default: @@ -655,7 +683,7 @@ marshal_4_octets (DBusString *str, static dbus_bool_t marshal_8_octets (DBusString *str, int insert_at, - DBusBasicValue value, + dbus_uint64_t value, int byte_order, int *pos_after) { @@ -803,16 +831,26 @@ _dbus_marshal_write_basic (DBusString *str, int byte_order, int *pos_after) { - const DBusBasicValue *vp; + /* Static assertions near the top of this file assert that signed and + * unsigned 16- and 32-bit quantities have the same alignment, and that + * doubles have alignment at least as strict as unsigned int64, so we + * don't have to distinguish further: every (double *) + * has strong enough alignment to be treated as though it was a + * (dbus_uint64_t *). Going via a (void *) means the compiler should + * know that pointers can alias each other. */ + const unsigned char *u8_p; + const dbus_uint16_t *u16_p; + const dbus_uint32_t *u32_p; + const dbus_uint64_t *u64_p; + const char * const *string_p; _dbus_assert (dbus_type_is_basic (type)); - vp = value; - switch (type) { case DBUS_TYPE_BYTE: - if (!_dbus_string_insert_byte (str, insert_at, vp->byt)) + u8_p = value; + if (!_dbus_string_insert_byte (str, insert_at, *u8_p)) return FALSE; if (pos_after) *pos_after = insert_at + 1; @@ -820,33 +858,39 @@ _dbus_marshal_write_basic (DBusString *str, break; case DBUS_TYPE_INT16: case DBUS_TYPE_UINT16: - return marshal_2_octets (str, insert_at, vp->u16, + u16_p = value; + return marshal_2_octets (str, insert_at, *u16_p, byte_order, pos_after); break; case DBUS_TYPE_BOOLEAN: - return marshal_4_octets (str, insert_at, vp->u32 != FALSE, + u32_p = value; + return marshal_4_octets (str, insert_at, (*u32_p != FALSE), byte_order, pos_after); break; case DBUS_TYPE_INT32: case DBUS_TYPE_UINT32: case DBUS_TYPE_UNIX_FD: - return marshal_4_octets (str, insert_at, vp->u32, + u32_p = value; + return marshal_4_octets (str, insert_at, *u32_p, byte_order, pos_after); break; case DBUS_TYPE_INT64: case DBUS_TYPE_UINT64: case DBUS_TYPE_DOUBLE: - return marshal_8_octets (str, insert_at, *vp, byte_order, pos_after); + u64_p = value; + return marshal_8_octets (str, insert_at, *u64_p, byte_order, pos_after); break; case DBUS_TYPE_STRING: case DBUS_TYPE_OBJECT_PATH: - _dbus_assert (vp->str != NULL); - return marshal_string (str, insert_at, vp->str, byte_order, pos_after); + string_p = value; + _dbus_assert (*string_p != NULL); + return marshal_string (str, insert_at, *string_p, byte_order, pos_after); break; case DBUS_TYPE_SIGNATURE: - _dbus_assert (vp->str != NULL); - return marshal_signature (str, insert_at, vp->str, pos_after); + string_p = value; + _dbus_assert (*string_p != NULL); + return marshal_signature (str, insert_at, *string_p, pos_after); break; default: _dbus_assert_not_reached ("not a basic type"); @@ -955,7 +999,7 @@ swap_array (DBusString *str, static dbus_bool_t marshal_fixed_multi (DBusString *str, int insert_at, - const DBusBasicValue *value, + const void *value, int n_elements, int byte_order, int alignment, @@ -1030,8 +1074,18 @@ _dbus_marshal_write_fixed_multi (DBusString *str, int byte_order, int *pos_after) { - const void* vp = *(const DBusBasicValue**)value; - + /* Static assertions near the top of this file assert that signed and + * unsigned 16- and 32-bit quantities have the same alignment, and that + * doubles have alignment at least as strict as unsigned int64, so we + * don't have to distinguish further: every (double *) + * has strong enough alignment to be treated as though it was a + * (dbus_uint64_t *). Going via a (void *) means the compiler should + * know that pointers can alias each other. */ + const unsigned char * const *u8_pp; + const dbus_uint16_t * const *u16_pp; + const dbus_uint32_t * const *u32_pp; + const dbus_uint64_t * const *u64_pp; + _dbus_assert (dbus_type_is_fixed (element_type)); _dbus_assert (n_elements >= 0); @@ -1043,21 +1097,25 @@ _dbus_marshal_write_fixed_multi (DBusString *str, switch (element_type) { case DBUS_TYPE_BYTE: - return marshal_1_octets_array (str, insert_at, vp, n_elements, byte_order, pos_after); + u8_pp = value; + return marshal_1_octets_array (str, insert_at, *u8_pp, n_elements, byte_order, pos_after); break; case DBUS_TYPE_INT16: case DBUS_TYPE_UINT16: - return marshal_fixed_multi (str, insert_at, vp, n_elements, byte_order, 2, pos_after); + u16_pp = value; + return marshal_fixed_multi (str, insert_at, *u16_pp, n_elements, byte_order, 2, pos_after); case DBUS_TYPE_BOOLEAN: case DBUS_TYPE_INT32: case DBUS_TYPE_UINT32: case DBUS_TYPE_UNIX_FD: - return marshal_fixed_multi (str, insert_at, vp, n_elements, byte_order, 4, pos_after); + u32_pp = value; + return marshal_fixed_multi (str, insert_at, *u32_pp, n_elements, byte_order, 4, pos_after); break; case DBUS_TYPE_INT64: case DBUS_TYPE_UINT64: case DBUS_TYPE_DOUBLE: - return marshal_fixed_multi (str, insert_at, vp, n_elements, byte_order, 8, pos_after); + u64_pp = value; + return marshal_fixed_multi (str, insert_at, *u64_pp, n_elements, byte_order, 8, pos_after); break; default: @@ -1465,7 +1523,7 @@ void _dbus_marshal_read_fixed_multi (const DBusString *str, int pos, int element_type, - void *value, + const void **value, int n_elements, int byte_order, int *new_pos) @@ -1487,7 +1545,7 @@ _dbus_marshal_read_fixed_multi (const DBusString *str, array_len = n_elements * alignment; - *(const DBusBasicValue**) value = (void*) _dbus_string_get_const_data_len (str, pos, array_len); + *value = _dbus_string_get_const_data_len (str, pos, array_len); if (new_pos) *new_pos = pos + array_len; } @@ -1573,7 +1631,8 @@ swap_test_array (void *array, int next; \ alignment = _dbus_type_get_alignment (DBUS_TYPE_##typename); \ v_UINT32 = _dbus_marshal_read_uint32 (&str, dump_pos, byte_order, &next); \ - _dbus_marshal_read_fixed_multi (&str, next, DBUS_TYPE_##typename, &v_ARRAY_##typename, \ + _dbus_marshal_read_fixed_multi (&str, next, DBUS_TYPE_##typename, \ + (const void **) &v_ARRAY_##typename, \ v_UINT32/alignment, \ byte_order, NULL); \ swap_test_array (v_ARRAY_##typename, v_UINT32, \ diff --git a/dbus/dbus-marshal-basic.h b/dbus/dbus-marshal-basic.h index 0c4e41172..15971b329 100644 --- a/dbus/dbus-marshal-basic.h +++ b/dbus/dbus-marshal-basic.h @@ -191,7 +191,7 @@ void _dbus_marshal_read_basic (const DBusString *str, void _dbus_marshal_read_fixed_multi (const DBusString *str, int pos, int element_type, - void *value, + const void **value, int n_elements, int byte_order, int *new_pos); diff --git a/dbus/dbus-marshal-recursive-util.c b/dbus/dbus-marshal-recursive-util.c index 145d2617f..f53e38278 100644 --- a/dbus/dbus-marshal-recursive-util.c +++ b/dbus/dbus-marshal-recursive-util.c @@ -2168,7 +2168,7 @@ uint16_read_multi (TestTypeNode *node, check_expected_type (reader, node->klass->typecode); _dbus_type_reader_read_fixed_multi (reader, - &values, + (const void **) &values, &n_elements); if (n_elements != count) @@ -2294,7 +2294,7 @@ uint32_read_multi (TestTypeNode *node, check_expected_type (reader, node->klass->typecode); _dbus_type_reader_read_fixed_multi (reader, - &values, + (const void **) &values, &n_elements); if (n_elements != count) diff --git a/dbus/dbus-marshal-recursive.c b/dbus/dbus-marshal-recursive.c index 417068c6a..6ad49ace1 100644 --- a/dbus/dbus-marshal-recursive.c +++ b/dbus/dbus-marshal-recursive.c @@ -922,7 +922,7 @@ _dbus_type_reader_get_array_length (const DBusTypeReader *reader) */ void _dbus_type_reader_read_fixed_multi (const DBusTypeReader *reader, - void *value, + const void **value, int *n_elements) { int element_type; @@ -956,12 +956,11 @@ _dbus_type_reader_read_fixed_multi (const DBusTypeReader *reader, _dbus_assert (remaining_len <= total_len); if (remaining_len == 0) - *(const DBusBasicValue**) value = NULL; + *value = NULL; else - *(const DBusBasicValue**) value = - (void*) _dbus_string_get_const_data_len (reader->value_str, - reader->value_pos, - remaining_len); + *value = _dbus_string_get_const_data_len (reader->value_str, + reader->value_pos, + remaining_len); *n_elements = remaining_len / alignment; _dbus_assert ((remaining_len % alignment) == 0); diff --git a/dbus/dbus-marshal-recursive.h b/dbus/dbus-marshal-recursive.h index c20bb913a..c3b96819d 100644 --- a/dbus/dbus-marshal-recursive.h +++ b/dbus/dbus-marshal-recursive.h @@ -119,7 +119,7 @@ void _dbus_type_reader_read_basic (const DBusTypeReader * int _dbus_type_reader_get_array_length (const DBusTypeReader *reader); DBUS_PRIVATE_EXPORT void _dbus_type_reader_read_fixed_multi (const DBusTypeReader *reader, - void *value, + const void **value, int *n_elements); void _dbus_type_reader_read_raw (const DBusTypeReader *reader, const unsigned char **value_location); diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index e43c4b3a5..edf76549f 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -893,9 +893,9 @@ _dbus_message_iter_get_args_valist (DBusMessageIter *iter, } else if (dbus_type_is_basic (spec_type)) { - DBusBasicValue *ptr; + void *ptr; - ptr = va_arg (var_args, DBusBasicValue*); + ptr = va_arg (var_args, void *); _dbus_assert (ptr != NULL); @@ -906,7 +906,7 @@ _dbus_message_iter_get_args_valist (DBusMessageIter *iter, { int element_type; int spec_element_type; - const DBusBasicValue **ptr; + const void **ptr; int *n_elements_p; DBusTypeReader array; @@ -928,7 +928,7 @@ _dbus_message_iter_get_args_valist (DBusMessageIter *iter, if (dbus_type_is_fixed (spec_element_type) && element_type != DBUS_TYPE_UNIX_FD) { - ptr = va_arg (var_args, const DBusBasicValue**); + ptr = va_arg (var_args, const void **); n_elements_p = va_arg (var_args, int*); _dbus_assert (ptr != NULL); @@ -936,8 +936,7 @@ _dbus_message_iter_get_args_valist (DBusMessageIter *iter, _dbus_type_reader_recurse (&real->u.reader, &array); - _dbus_type_reader_read_fixed_multi (&array, - (void *) ptr, n_elements_p); + _dbus_type_reader_read_fixed_multi (&array, ptr, n_elements_p); } else if (_DBUS_TYPE_IS_STRINGLIKE (spec_element_type)) { @@ -1060,7 +1059,7 @@ _dbus_message_iter_get_args_valist (DBusMessageIter *iter, else if (dbus_type_is_basic (spec_type)) { /* move the index forward */ - va_arg (copy_args, DBusBasicValue *); + va_arg (copy_args, const void *); } else if (spec_type == DBUS_TYPE_ARRAY) { @@ -1070,7 +1069,7 @@ _dbus_message_iter_get_args_valist (DBusMessageIter *iter, if (dbus_type_is_fixed (spec_element_type)) { /* move the index forward */ - va_arg (copy_args, const DBusBasicValue **); + va_arg (copy_args, const void **); va_arg (copy_args, int *); } else if (_DBUS_TYPE_IS_STRINGLIKE (spec_element_type)) @@ -1872,8 +1871,8 @@ dbus_message_append_args_valist (DBusMessage *message, { if (dbus_type_is_basic (type)) { - const DBusBasicValue *value; - value = va_arg (var_args, const DBusBasicValue*); + const void *value; + value = va_arg (var_args, const void *); if (!dbus_message_iter_append_basic (&iter, type, @@ -1899,10 +1898,10 @@ dbus_message_append_args_valist (DBusMessage *message, if (dbus_type_is_fixed (element_type) && element_type != DBUS_TYPE_UNIX_FD) { - const DBusBasicValue **value; + const void **value; int n_elements; - value = va_arg (var_args, const DBusBasicValue**); + value = va_arg (var_args, const void **); n_elements = va_arg (var_args, int); if (!dbus_message_iter_append_fixed_array (&array,