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

va_start behavior in uv_loop_configure is undefined. #978

Closed
hokein opened this Issue Aug 5, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@hokein
Copy link

hokein commented Aug 5, 2016

there is a a -Wvarargs warning when compiling libuv with clang.

uv/src/uv-common.c:572:16: warning: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Wvarargs]
  va_start(ap, option);

Note that option type is uv_loop_option which is an enum. This violates the promotion rules
for passing the value to va_start, so the behavior of va_start is undefined here. From C standard 7.16.1.4:

The parameter parmN is the identifier of the rightmost parameter in the variable parameter list in the function definition (the one just before the ...). If the parameter parmN is declared with the register storage class, with a function or array type, or with a type that is not compatible with the type that results after application of the default argument promotions, the behavior is undefined.

For more details, you can refer to https://www.securecoding.cert.org/confluence/display/cplusplus/EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Aug 6, 2016

I assume the fix is to rewrite it to va_start(ap, (int) option)? It would take a very sadistic compiler to turn the current code into actual UB though, IMO.

Out of curiosity, with what clang version is that? 3.9.0?

@hokein

This comment has been minimized.

Copy link
Author

hokein commented Aug 6, 2016

@bnoordhuis Thanks for the reply.

va_start(ap, (int) option) isn't a correct fix, IMO. The promotion rule is still in va_start. I don't know whether there is a fix without changing uv_loop_option interface.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Aug 6, 2016

Can you explain when it would make a material difference? sizeof(uv_loop_option) == sizeof(int) and they're both integral types so I'm having a hard time seeing how default argument promotion could turn bad. Clang's warning borders on language lawyering, IMO.

I don't know whether there is a fix without changing uv_loop_option interface.

That would break source compatibility so it's not an option for the v1.x branch but it could be considered for the v2.x branch.

@ntd

This comment has been minimized.

Copy link

ntd commented Dec 25, 2016

sizeof(uv_loop_option) == sizeof(int)

uv_loop_option is an enum and the compiler can choose to use a short (or even a char) as internal representation, so the above assumption can be wrong. For instance, even with gcc, you will likely break the build by enabling -fshort-enums.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Apr 12, 2018

That particular warning seems to have been taken out of clang again and we don't build with -fshort-enums nor are there compilers in our support list where sizeof(uv_loop_option) == sizeof(int) doesn't hold.

No action appears necessary at this time so I'm going to close this out. Please leave a comment or open a pull request if this affects you.

@bnoordhuis bnoordhuis closed this Apr 12, 2018

PeterJohnson added a commit to PeterJohnson/allwpilib that referenced this issue Jun 17, 2018

PeterJohnson added a commit to PeterJohnson/allwpilib that referenced this issue Jun 18, 2018

PeterJohnson added a commit to PeterJohnson/libuv that referenced this issue Jun 18, 2018

PeterJohnson added a commit to PeterJohnson/allwpilib that referenced this issue Jun 18, 2018

niksu added a commit to niksu/tym that referenced this issue Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment