Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 24, 2016

Conversation

darconeous
Copy link
Contributor

@darconeous darconeous commented Jun 24, 2016

@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
and the caller must ONLY call va_end() on the object.

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

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! 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().

@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
@darconeous darconeous added this to the 0.01.00 (Initial Official Release) milestone Jun 24, 2016
@darconeous
Copy link
Contributor Author

Tagging @abtink, because he'll probably get a kick out of this. :P

@abtink
Copy link
Member

abtink commented Jun 24, 2016

Very cool! Thanks.

@jwhui
Copy link
Member

jwhui commented Jun 24, 2016

LGTM 👍

@jwhui jwhui merged commit a66b6fc into openthread:master Jun 24, 2016
vaas-krish pushed a commit to vaas-krish/openthread that referenced this pull request May 23, 2017
…ub_75358c5 to master

* commit '765afb2adffa11ef25002642e8fa2961c6685f5d': (25 commits)
  CLI: enhance ping command (openthread#211)
  Add note/comment to describe the behavior of otAddUnicastAddress() (openthread#220)
  Update to reflect new binary name. (openthread#221)
  Fix replay protection. (openthread#218)
  ExternalRouteLookup(): Use local var in inner loop iterating over sub-tlvs (openthread#217)
  Unify example apps across all example platforms. (openthread#215)
  Enhance the calculation of link quality metrics (openthread#197)
  Remove redefinition of OT_NETWORK_NAME_SIZE. (openthread#212)
  Add ability to configure whether or not OpenThread handles ICMPv6 Echo Request/Reply messages. (openthread#207)
  Add test for 32-bit target. (openthread#209)
  spinel: Don't rely on unspecified va_list behavior (openthread#208)
  ncp-spinel: Add support for `PROP_THREAD_ASSISTING_PORTS` (openthread#205)
  ncp-spinel: Avoid using ssize_t, provide fallbacks for errno defines (openthread#203)
  Fix bug in reading IEEE 802.15.4 Extended Address from a MAC header. (openthread#201)
  ncp-spinel: Updates to `PROTOCOL.md` (openthread#198)
  Add ability to configure unsecure ports to the IPv6 datagram filter. (openthread#196)
  Fix astyle wrap script. (openthread#195)
  Fix code style. (openthread#194)
  ncp-spinel: Remove include <sys/types.h> (openthread#193)
  Update pretty and pretty-check make variables. (openthread#192)
  ...
vaas-krish pushed a commit to vaas-krish/openthread that referenced this pull request May 23, 2017
…ub_69180fc to master

* commit '05972faf0d5766c5474c302d76803235b842d341':
  Update DA15000 Readme with Trobleshooting section (openthread#1112)
  Send null/empty frame in response to data poll from a child with no queued frame (openthread#701)
  Minor cleanup of DA15000 build files. (openthread#1111)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants