espressif/openthread
1
0
Fork 0
mirror of https://github.com/espressif/openthread.git synced 2026-08-02 17:17:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

spinel: Don't rely on unspecified va_list behavior (#208)

Browse Source 
@jwhui noticed a problem when he set up the 32-bit-specific travis
build: the Spinel unit test was failing. Further investigation
indicated that the existing code was relying on some undefined
behavior of the 64-bit x86 implementation of `va_list`.

It turns out that if you pass a `va_list` by value through a function,
[the exact state of the `va_list` object is undefined after that call
returns][1] and the caller must ONLY call `va_end()` on the object.

From the [C99 standard, section 7.15, bullet 3 on page 249][2]:

> The object `ap` may be passed as an argument to another function; if
> that function invokes the `va_arg` macro with parameter `ap`, the
> value of `ap` in the calling function is indeterminate and shall be
> passed to the `va_end` macro prior to any further reference to
> `ap`.²¹⁵

And footnote 215 even says:

> It is permitted to create a pointer to a `va_list` and pass that
> pointer to another function, in which case the original function may
> make further use of the original list after the other function
> returns

Sounds great! So we should just pass around a pointer, right?

[**BZZZT!**][3] Turns out that on x86 platforms, `va_list` is actually
an array, and as you should already know arrays are treated passed by
pointer (instead of by value) when you pass them as function
arguments.

The solution is to put the `va_list` in a struct, use `va_copy()` to
initialize it, and then pass that struct around by pointer instead.
This guarantees that we get the behavior we were relying on, at the
expense of an occasional extra call to `va_copy()`.

[1]: http://stackoverflow.com/questions/3369588/pass-va-list-or-pointer-to-va-list
[2]: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf#page-249
[3]: http://stackoverflow.com/questions/8047362/is-gcc-mishandling-a-pointer-to-a-va-list-passed-to-a-function
This commit is contained in:
Robert Quattlebaum
2016-06-23 18:46:08 -07:00
committed by Jonathan Hui
parent a080779b1e
commit a66b6fc6b7
2 changed files with 85 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ncp/spinel.c
+85 -52
View File
@@ -46,9 +46,9 @@
#include "spinel.h"
#include <assert.h>
#include <string.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
// ----------------------------------------------------------------------------
@@ -103,6 +103,11 @@ static int spinel_errno_workaround_;
#define require(c, l) require_action(c, l, {})
#endif
typedef struct {
va_list obj;
} va_list_obj;
// ----------------------------------------------------------------------------
// MARK: -
@@ -217,21 +222,8 @@ spinel_next_packed_datatype(const char *pack_format)
return pack_format;
}
spinel_ssize_t
spinel_datatype_unpack(const uint8_t *data_ptr, spinel_size_t data_len, const char *pack_format, ...)
{
spinel_ssize_t ret;
va_list args;
va_start(args, pack_format);
ret = spinel_datatype_vunpack(data_ptr, data_len, pack_format, args);
va_end(args);
return ret;
}
spinel_ssize_t
spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const char *pack_format, va_list args)
static spinel_ssize_t
spinel_datatype_vunpack_(const uint8_t *data_ptr, spinel_size_t data_len, const char *pack_format, va_list_obj *args)
{
spinel_ssize_t ret = 0;
@@ -247,7 +239,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
{
case SPINEL_DATATYPE_BOOL_C:
{
bool *arg_ptr = va_arg(args, bool *);
bool *arg_ptr = va_arg(args->obj, bool *);
require_action(data_len >= sizeof(uint8_t), bail, (ret = -1, errno = EOVERFLOW));
if (arg_ptr)
@@ -264,7 +256,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_INT8_C:
case SPINEL_DATATYPE_UINT8_C:
{
uint8_t *arg_ptr = va_arg(args, uint8_t *);
uint8_t *arg_ptr = va_arg(args->obj, uint8_t *);
require_action(data_len >= sizeof(uint8_t), bail, (ret = -1, errno = EOVERFLOW));
if (arg_ptr)
@@ -281,7 +273,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_INT16_C:
case SPINEL_DATATYPE_UINT16_C:
{
uint16_t *arg_ptr = va_arg(args, uint16_t *);
uint16_t *arg_ptr = va_arg(args->obj, uint16_t *);
require_action(data_len >= sizeof(uint16_t), bail, (ret = -1, errno = EOVERFLOW));
if (arg_ptr)
@@ -298,7 +290,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_INT32_C:
case SPINEL_DATATYPE_UINT32_C:
{
uint32_t *arg_ptr = va_arg(args, uint32_t *);
uint32_t *arg_ptr = va_arg(args->obj, uint32_t *);
require_action(data_len >= sizeof(uint32_t), bail, (ret = -1, errno = EOVERFLOW));
if (arg_ptr)
@@ -314,7 +306,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_IPv6ADDR_C:
{
spinel_ipv6addr_t **arg_ptr = va_arg(args, spinel_ipv6addr_t **);
spinel_ipv6addr_t **arg_ptr = va_arg(args->obj, spinel_ipv6addr_t **);
require_action(data_len >= sizeof(spinel_ipv6addr_t), bail, (ret = -1, errno = EOVERFLOW));
if (arg_ptr)
@@ -330,7 +322,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_EUI64_C:
{
spinel_eui64_t **arg_ptr = va_arg(args, spinel_eui64_t **);
spinel_eui64_t **arg_ptr = va_arg(args->obj, spinel_eui64_t **);
require_action(data_len >= sizeof(spinel_eui64_t), bail, (ret = -1, errno = EOVERFLOW));
if (arg_ptr)
@@ -346,7 +338,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_EUI48_C:
{
spinel_eui48_t **arg_ptr = va_arg(args, spinel_eui48_t **);
spinel_eui48_t **arg_ptr = va_arg(args->obj, spinel_eui48_t **);
require_action(data_len >= sizeof(spinel_eui48_t), bail, (ret = -1, errno = EOVERFLOW));
if (arg_ptr)
@@ -362,7 +354,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_UINT_PACKED_C:
{
unsigned int *arg_ptr = va_arg(args, unsigned int *);
unsigned int *arg_ptr = va_arg(args->obj, unsigned int *);
spinel_ssize_t pui_len = spinel_packed_uint_decode(data_ptr, data_len, arg_ptr);
require(pui_len > 0, bail);
@@ -377,7 +369,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
case SPINEL_DATATYPE_UTF8_C:
{
const char **arg_ptr = va_arg(args, const char **);
const char **arg_ptr = va_arg(args->obj, const char **);
size_t len = strnlen((const char *)data_ptr, data_len) + 1;
require_action((len <= data_len) || (data_ptr[data_len - 1] != 0), bail, (ret = -1, errno = EOVERFLOW));
@@ -398,8 +390,8 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
spinel_ssize_t pui_len = 0;
uint16_t block_len = 0;
const uint8_t *block_ptr = data_ptr;
const uint8_t **block_ptr_ptr = va_arg(args, const uint8_t **);
unsigned int *block_len_ptr = va_arg(args, unsigned int *);
const uint8_t **block_ptr_ptr = va_arg(args->obj, const uint8_t **);
unsigned int *block_len_ptr = va_arg(args->obj, unsigned int *);
char nextformat = *spinel_next_packed_datatype(pack_format);
if ((nextformat != 0) && (nextformat != ')'))
@@ -460,7 +452,7 @@ spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const c
require_action(data_len >= (block_len + pui_len), bail, (ret = -1, errno = EOVERFLOW));
actual_len = spinel_datatype_vunpack(block_ptr, block_len, pack_format + 2, args);
actual_len = spinel_datatype_vunpack_(block_ptr, block_len, pack_format + 2, args);
require_action((int)actual_len > -1, bail, (ret = -1, errno = EOVERFLOW));
@@ -499,20 +491,33 @@ bail:
}
spinel_ssize_t
spinel_datatype_pack(uint8_t *data_ptr, spinel_size_t data_len_max, const char *pack_format, ...)
spinel_datatype_unpack(const uint8_t *data_ptr, spinel_size_t data_len, const char *pack_format, ...)
{
int ret;
va_list args;
va_start(args, pack_format);
spinel_ssize_t ret;
va_list_obj args;
va_start(args.obj, pack_format);
ret = spinel_datatype_vpack(data_ptr, data_len_max, pack_format, args);
ret = spinel_datatype_vunpack_(data_ptr, data_len, pack_format, &args);
va_end(args);
va_end(args.obj);
return ret;
}
spinel_ssize_t
spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char *pack_format, va_list args)
spinel_datatype_vunpack(const uint8_t *data_ptr, spinel_size_t data_len, const char *pack_format, va_list args)
{
spinel_ssize_t ret;
va_list_obj args_obj;
va_copy(args_obj.obj, args);
ret = spinel_datatype_vunpack_(data_ptr, data_len, pack_format, &args_obj);
va_end(args_obj.obj);
return ret;
}
static spinel_ssize_t
spinel_datatype_vpack_(uint8_t *data_ptr, spinel_size_t data_len_max, const char *pack_format, va_list_obj *args)
{
spinel_ssize_t ret = 0;
@@ -528,7 +533,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
{
case SPINEL_DATATYPE_BOOL_C:
{
bool arg = va_arg(args, int);
bool arg = (bool)va_arg(args->obj, int);
ret += sizeof(uint8_t);
if (data_len_max >= sizeof(uint8_t))
@@ -548,7 +553,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_INT8_C:
case SPINEL_DATATYPE_UINT8_C:
{
uint8_t arg = va_arg(args, int);
uint8_t arg = (uint8_t)va_arg(args->obj, int);
ret += sizeof(uint8_t);
if (data_len_max >= sizeof(uint8_t))
@@ -568,7 +573,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_INT16_C:
case SPINEL_DATATYPE_UINT16_C:
{
uint16_t arg = va_arg(args, int);
uint16_t arg = (uint16_t)va_arg(args->obj, int);
ret += sizeof(uint16_t);
if (data_len_max >= sizeof(uint16_t))
@@ -589,7 +594,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_INT32_C:
case SPINEL_DATATYPE_UINT32_C:
{
uint32_t arg = va_arg(args, int);
uint32_t arg = (uint32_t)va_arg(args->obj, int);
ret += sizeof(uint32_t);
if (data_len_max >= sizeof(uint32_t))
@@ -611,7 +616,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_IPv6ADDR_C:
{
spinel_ipv6addr_t *arg = va_arg(args, spinel_ipv6addr_t *);
spinel_ipv6addr_t *arg = va_arg(args->obj, spinel_ipv6addr_t *);
ret += sizeof(spinel_ipv6addr_t);
if (data_len_max >= sizeof(spinel_ipv6addr_t))
@@ -630,7 +635,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_EUI48_C:
{
spinel_eui48_t *arg = va_arg(args, spinel_eui48_t *);
spinel_eui48_t *arg = va_arg(args->obj, spinel_eui48_t *);
ret += sizeof(spinel_eui48_t);
if (data_len_max >= sizeof(spinel_eui48_t))
@@ -649,7 +654,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_EUI64_C:
{
spinel_eui64_t *arg = va_arg(args, spinel_eui64_t *);
spinel_eui64_t *arg = va_arg(args->obj, spinel_eui64_t *);
ret += sizeof(spinel_eui64_t);
if (data_len_max >= sizeof(spinel_eui64_t))
@@ -668,7 +673,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_UINT_PACKED_C:
{
uint32_t arg = va_arg(args, uint32_t);
uint32_t arg = va_arg(args->obj, uint32_t);
spinel_ssize_t encoded_size = spinel_packed_uint_encode(data_ptr, data_len_max, arg);
ret += encoded_size;
@@ -687,7 +692,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_UTF8_C:
{
const char *string_arg = va_arg(args, const char *);
const char *string_arg = va_arg(args->obj, const char *);
size_t string_arg_len = 0;
if (string_arg)
@@ -719,8 +724,8 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
case SPINEL_DATATYPE_DATA_C:
{
uint8_t *arg = va_arg(args, uint8_t *);
uint32_t data_size_arg = va_arg(args, uint32_t);
const uint8_t *arg = va_arg(args->obj, const uint8_t *);
uint32_t data_size_arg = va_arg(args->obj, uint32_t);
spinel_ssize_t size_len = 0;
char nextformat = *spinel_next_packed_datatype(pack_format);
@@ -760,10 +765,10 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
// First we figure out the size of the struct
{
va_list subargs;
va_copy(subargs, args);
struct_len = spinel_datatype_vpack(NULL, 0, pack_format + 2, subargs);
va_end(subargs);
va_list_obj subargs;
va_copy(subargs.obj, args->obj);
struct_len = spinel_datatype_vpack_(NULL, 0, pack_format + 2, &subargs);
va_end(subargs.obj);
}
if (nextformat != 0 && nextformat != ')')
@@ -779,7 +784,7 @@ spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char
data_ptr += size_len;
data_len_max -= size_len;
struct_len = spinel_datatype_vpack(data_ptr, data_len_max, pack_format + 2, args);
struct_len = spinel_datatype_vpack_(data_ptr, data_len_max, pack_format + 2, args);
data_ptr += struct_len;
data_len_max -= struct_len;
@@ -809,6 +814,34 @@ bail:
return ret;
}
spinel_ssize_t
spinel_datatype_pack(uint8_t *data_ptr, spinel_size_t data_len_max, const char *pack_format, ...)
{
int ret;
va_list_obj args;
va_start(args.obj, pack_format);
ret = spinel_datatype_vpack_(data_ptr, data_len_max, pack_format, &args);
va_end(args.obj);
return ret;
}
spinel_ssize_t
spinel_datatype_vpack(uint8_t *data_ptr, spinel_size_t data_len_max, const char *pack_format, va_list args)
{
int ret;
va_list_obj args_obj;
va_copy(args_obj.obj, args);
ret = spinel_datatype_vpack_(data_ptr, data_len_max, pack_format, &args_obj);
va_end(args_obj.obj);
return ret;
}
// ----------------------------------------------------------------------------
// MARK: -
src/ncp/spinel.h
-1
View File
@@ -29,7 +29,6 @@
#define SPINEL_HEADER_INCLUDED 1
#include <stdarg.h>
#include <stdio.h>
#include <stdbool.h>
#include <stdint.h>