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

Potentially insufficient checks for undefined behavior in C++ API #22940

Open
kevle opened this issue Nov 25, 2024 · 2 comments
Open

Potentially insufficient checks for undefined behavior in C++ API #22940

kevle opened this issue Nov 25, 2024 · 2 comments

Comments

@kevle
Copy link

kevle commented Nov 25, 2024

Describe the issue

At the moment, the C++ API might insufficiently check for potential undefined behavior when using reinterpret_cast on wrapper types.
See e.g. this piece of code to run inference:

template <typename T>
inline void SessionImpl<T>::Run(const RunOptions& run_options, const char* const* input_names, const Value* input_values, size_t input_count,
const char* const* output_names, Value* output_values, size_t output_count) {
static_assert(sizeof(Value) == sizeof(OrtValue*), "Value is really just an array of OrtValue* in memory, so we can reinterpret_cast safely");
auto ort_input_values = reinterpret_cast<const OrtValue* const*>(input_values);
auto ort_output_values = reinterpret_cast<OrtValue**>(output_values);
ThrowOnError(GetApi().Run(this->p_, run_options, input_names, ort_input_values, input_count, output_names, output_count, ort_output_values));
}

The accompanying static_assert claims that the check verifies that :

"Value is really just an array of OrtValue* in memory, so we can reinterpret_cast safely"

I do not think this check sizeof(Value) == sizeof(OrtValue*) is a universally sufficient condition to verify safe casting. Checking for size of the types is only one part of the requirements on safe reinterpret_cast use. The types are neither checked for compatible alignment nor is the C++ wrapper type checked for standard layout compliance.

I have tried to find compilation tests that might check those properties in CI, but my search came up empty so far.
At the moment there does not seem to be a problem, but I think adding those checks somewhere might help with robustness of the wrapper going forward, even if they are still not 100% conclusive in determining whether there is UB involved in the cast.

To reproduce

Look at include/onnxruntime/core/session/onnxruntime_cxx_inline.h lines 1065++

Urgency

Not particular urgent.

Platform

Other / Unknown

OS Version

Any

ONNX Runtime Installation

Released Package

ONNX Runtime Version or Commit ID

09d2ee6

ONNX Runtime API

C++

Architecture

X64

Execution Provider

Default CPU

Execution Provider Library Version

No response

@fs-eire
Copy link
Contributor

fs-eire commented Nov 26, 2024

the C++ API might insufficiently check for potential undefined behavior when using reinterpret_cast on wrapper types.

  • C++ is not a type-safe language. In theory, C++ code can cast any unsigned integer to a pointer to any type. There is no way to guarantee input_values/output_values are of correct type by checks.
  • If users use the ORT C++ API normally (without arbitrary reinterpret_cast or C-style cast), the C++ API classes (eg. Value/Session classes) are type safe by design. This is because the implementation of Value class is using the value of itself as a pointer to OrtValue (the opaque pointer design pattern).

@skottmckay
Copy link
Contributor

How would you suggest we test it? The C++ API is really a thin convenience wrapper over the C API. The 'real' ORT data types are all opaque at the API level so I'm not sure what additional checks can be done here.

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