OpenSSF silver badge: fix the issues on hardening

[mgovers]

in #1241, hardening was enabled. Clang actually raised some valid undefined behavior issues:

/power-grid-model/power_grid_model_c/power_grid_model_cpp/include/power_grid_model_cpp/handle.hpp:95:27: runtime error: call to function PGM_dataset_foo through pointer to incorrect function type 'const PGM_ConstDataset *(*)(PGM_Handle *, long)'
/power-grid-model/power_grid_model_c/power_grid_model_c/src/dataset.cpp: note: PGM_dataset_foo defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /power-grid-model/power_grid_model_c/power_grid_model_cpp/include/power_grid_model_cpp/handle.hpp:95:27

The underlying reason was that we do

#ifndef PGM_DLL_EXPORTS
typedef struct PGM_ConstDataset PGM_ConstDataset;
// ...
#endif

// ...
PGM_ConstDataset* PGM_create_dataset_const (/*...*/);
void              PGM_destroy_dataset_const(PGM_ConstDataset* /*...*/);
PGM_Foo           PGM_dataset_foo          (PGM_ConstDataset* /*...*/);

and the C API implementation then contains

#define PGM_DLL_EXPORTS
#include "power_grid_model_c/basics.h"

#include <power_grid_model/auxiliary/dataset.hpp>

using PGM_ConstDataset = power_grid_model::meta_data::ConstDataset;
// ...

PGM_ConstDataset* PGM_create_dataset_const(/*...*/) { /* ... */ }
PGM_destroy_dataset_const(PGM_ConstDataset* /*...*/) { /* ... */ }
PGM_Foo* PGM_dataset_foo(PGM_ConstDataset /*...*/) { /* ... */ }

but the user will import like

#include "power_grid_model_c/basics.h"

PGM_ConstDataset dataset = PGM_create_dataset_const(/*...*/);
PGM_dataset_foo(dataset);
PGM_destroy_dataset_const(dataset);

This effectively means that the C API implementation will see a type

using PGM_ConstDataset = power_grid_model::meta_data::ConstDataset;
using PGM_DatasetInfo = power_grid_model::meta_data::DatasetInfo;
// ...

power_grid_model::meta_data::ConstDataset* PGM_create_dataset_const(/*...*/) { /* ... */ }
PGM_destroy_dataset_const(power_grid_model::meta_data::ConstDataset* /*...*/) { /* ... */ }
PGM_Foo PGM_dataset_foo(power_grid_model::meta_data::ConstDataset* /*...*/) { /* ... */ }

while the user will see a type

typedef struct PGM_ConstDataset PGM_ConstDataset;

PGM_ConstDataset* PGM_create_dataset_const(/*...*/);
PGM_destroy_dataset_const(PGM_ConstDataset* /*...*/);

// usage
PGM_ConstDataset dataset = PGM_create_dataset_const(/*...*/);
PGM_dataset_foo(dataset);
PGM_destroy_dataset_const(dataset);

Or, in short, the user will see a type struct PGM_ConstDataset that is its own type, but the implementation will see the type power_grid_model::meta_data::ConstDatast.

After digging into it, it seemed very similar to something that was found in the CPython implementation (python/cpython#111178). I then found that we do not have such issues for PGM_PowerGridModel, the implementation of which is defined as struct PGM_PowerGridModel: public power_grid_model::MainModel {};, which IS, in fact, its own type, rather than a type alias that comes from a using declaration.

The reason a using declaration was used in the first place is so that we could apply forward declarations, but now it seems that this was not the right solution. Note as well that the #ifndef PGM_DLL_EXPORT guard was used in the first place to suppress the exact same compiler errors, but we did not realize that the same undefined behavior was still being invoked on the user side.

To solve the issue in a way that still allows support for both features, I ended up following a similar approach as to main model, by using a derived type implementation, but also added forward declared, statically linked wrap and unwrap functions to handle the type conversions for files that use the forward declared version.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[TonyXiang8787]

What undefined behavior we are talking about? What we do is a well-known practice for many libraries, to declare an opaque type in C-API user side. But in C-API implementation side, we surpress that opaque type, and instead directly have a using directive to refer to a concrete type inside namespace.

[TonyXiang8787]

Hi @mgovers, after some digging I see this is indeed UB. Not because of pointer case, which is well-defined if you do round-trip opaque pointer cast and never dereference the opaque pointer (you can't anyway). But because the function signature has different types between translation units.

But I do not think using wrapper type (even for MainModel) is an appropriate solution. A reintepret_cast should be more justified here because that's exactly what we are doing: define opaque type at user side, and reintepret_cast at implementation side to concrete types. Note in C++, cast from CPPType* to CAPIType*, and cast back CAPIType* to CPPType* and dereference it is well-defined as long as you never dereference CAPIType*, which we don't because the type is opaque.

So I would propose to just pre-define a reintepret_cast list, like this:

template<class c_type, class cpp_type>
struct type_matching;

template<class...>
struct type_matching_list_impl;

using type_matching_list = typematching_list<
    type_matching<PGM_MetaComponent, power_grid_model::meta_data::MetaComponent>,
    // list all opaque to concrete type matching
>;

template<class c_type_pointer>
using cpp_type_pointer_t = // some template magic to find the correct cpp type pointer, also keep const correctness

template<class c_type_pointer>
auto cast_to_cpp(c_type_pointer ptr) { 
return reintepret_cast<cpp_type_pointer_t<c_type_pointer>>(ptr); 
}

template<class cpp_type_pointer>
using c_type_pointer_t = // some template magic to find the correct c type pointer, also keep const correctness

template<class cpp_type_pointer>
auto cast_to_c(cpp_type_pointer ptr) { 
return reintepret_cast<c_type_pointer_t<cpp_type_pointer>>(ptr); 
}

After this, we can

1. remove the macro guard of those opaque times. Then in translation unit of our C-API compilation and user side they see the same funciton signature.
2. In cpp side of C-API implementation, call cast_to_cpp when you receive a C opaque type, and call cast_to_c when you want to return as C opaque type.

[mgovers]

So I would propose to just pre-define a reintepret_cast list, like this

I thought about something along those lines as well, but then we still have the typedef struct PGM_Foo PGM_Foo; forward declaration that needs to be defined somewhere. What would that definition be? Would you change it to typedef void PGM_Foo? Wouldn't that be a breaking change?

[TonyXiang8787]

So I would propose to just pre-define a reintepret_cast list, like this

I thought about something along those lines as well, but then we still have the typedef struct PGM_Foo PGM_Foo; forward declaration that needs to be defined somewhere. What would that definition be? Would you change it to typedef void PGM_Foo? Wouldn't that be a breaking change?

That's not needed. We do not need to define a forward opaque type. We can just cast between PGM_Foo* and power_grid_model::Foo*. The PGM_Foo is always opaque type, never defined.

[mgovers]

So I would propose to just pre-define a reintepret_cast list, like this

I thought about something along those lines as well, but then we still have the typedef struct PGM_Foo PGM_Foo; forward declaration that needs to be defined somewhere. What would that definition be? Would you change it to typedef void PGM_Foo? Wouldn't that be a breaking change?

That's not needed. We do not need to define a forward opaque type. We can just cast between PGM_Foo* and power_grid_model::Foo*. The PGM_Foo is always opaque type, never defined.

let me try