Skip to content

Miri is confused by a variadic call in px4 #2191

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

Closed
saethlin opened this issue Jun 5, 2022 · 6 comments
Closed

Miri is confused by a variadic call in px4 #2191

saethlin opened this issue Jun 5, 2022 · 6 comments

Comments

@saethlin
Copy link
Member

saethlin commented Jun 5, 2022

Yay more confusion with variadics

https://miri.saethlin.dev/ub?crate=px4&version=0.2.4

error: Undefined Behavior: calling a c-variadic function via a non-variadic call site, or vice versa
  --> /root/build/src/logging.rs:21:3
   |
21 | /         px4_log_raw(
22 | |             level as i32,
23 | |             "%.*s\0".as_ptr(),
24 | |             message.len() as i32,
25 | |             message.as_ptr(),
26 | |         );
   | |_________^ calling a c-variadic function via a non-variadic call site, or vice versa
   |

I don't know what a variadic call site means. But this looks like a perfectly valid call to me, all the variadic arguments fit in a usize. It's not possible to type check a variadic call without knowing how the callee is going to interpret things, so I feel like we should either be quiet on the issue or reject them altogether?

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2022

This means that a function was declared as variadic, but called through a function pointer that is not variadic (or vice versa). As far as I know, there is no cross-platform promise that this will actually work (though it might work for some specific platform ABIs).

But anyway, even if it passed this check, it would probably immediately run into the next check, which is that Miri does not support Rust-to-Rust calls of variadic functions. So, closing as a duplicate of #1892.

@RalfJung RalfJung closed this as not planned Won't fix, can't repro, duplicate, stale Jun 5, 2022
@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2022

Until we support variadics better, we can at least make the error less scary: rust-lang/rust#97767

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2022

What I do not understand is why we are even getting to that check. Looking at the source, this is just calling an extern function that Miri does not support. So I would expect a corresponding error, and on the playground, exactly that happens.

My only explanation is that something somewhere is defining a Rust function with link_name px4_log_raw, but I have not found that definition yet.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2022

Ah, here it is. So that test passes a bunch of arguments to a function that expects none. Is it a guarantee of all calling conventions that passing extra arguments is fine?

@bjorn3
Copy link
Member

bjorn3 commented Jun 5, 2022

It is UB according to the C specification. Rustc warns about clashing function declarations within the same crate. (and cg_clif simply crashes on cranelift returning an error from declare_function) It may be UB at llvm ir level. With CFI enabled the calling convention would probably reject the extra arguments. As for calling conventions when CFI is disabled, I'm not sure.

@saethlin
Copy link
Member Author

saethlin commented Jun 5, 2022

Ouch, I totally missed that declaration. I think it is definitely valid for Miri to report a problem here. This declaration vs definition issue is something C++ was trying to deal with in the 80s with name mangling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants