Check cwrap types when calling via pybind#1191
Open
eivindjahren wants to merge 3 commits into
Open
Conversation
6500bfd to
62601e0
Compare
There was a problem hiding this comment.
Pull request overview
This PR centralizes the pybind “cwrap handle → native pointer” conversion logic and adds runtime type-checking to prevent undefined behavior when Python callers pass incorrect object types into pybind/C++ entrypoints.
Changes:
- Introduces a shared
from_cwrap<T>(handle)implementation that validates Python object types before extracting_BaseCClass__c_pointer. - Updates
_grid,_kw,_rd_sum, and_fortiopybind modules to use the shared helper and adds an optional-kw variant for nullable keyword parameters. - Adds/updates Python tests to assert
TypeErroris raised for wrong-type inputs instead of risking UB.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rd_tests/test_rd_sum.py | Adds tests ensuring wrong-type keywords / time_points raise TypeError. |
| tests/rd_tests/test_rd_grid_misc.py | Switches to public Grid.load_column and adds wrong-type argument TypeError tests. |
| tests/rd_tests/test_grid.py | Adds Grid.create wrong-type argument TypeError tests and verifies None is accepted where intended. |
| lib/resdata/rd_sum_pybind.cpp | Replaces local cwrap casting helper with shared implementation. |
| lib/resdata/rd_kw_pybind.cpp | Replaces local cwrap casting helper with shared implementation. |
| lib/resdata/rd_grid_pybind.cpp | Replaces local cwrap casting helper with shared implementation and uses nullable kw conversion for actnum/mapaxes. |
| lib/resdata/fortio_pybind.cpp | Replaces local cwrap casting helper with shared implementation. |
| lib/resdata/cwrap_pybind.cpp | New shared implementation providing typed from_cwrap<T> specializations. |
| lib/private-include/detail/resdata/cwrap_pybind.hpp | New header exposing from_cwrap<T> and optional-kw helper to pybind modules. |
| lib/CMakeLists.txt | Adds cwrap_pybind.cpp to all relevant pybind extension modules and updates include paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5
to
+8
| #include <resdata/rd_kw.hpp> | ||
|
|
||
| template <typename T> T *from_cwrap(pybind11::handle obj); | ||
| rd_kw_type *from_cwrap_opt_kw(pybind11::handle obj); |
62601e0 to
a452404
Compare
ajaust
approved these changes
Jun 15, 2026
ajaust
left a comment
Contributor
There was a problem hiding this comment.
Nice changes! I only have a single point that might need validation (about changing some target_include_directories).
a452404 to
9447a0b
Compare
9447a0b to
fa80cbb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
TypeErrorfor calls to methods that would lead to undefined behavior.In order for this to not be a breaking change, we have to be sure that the previous behavior was not defined, at least not in documentation.
The simplest case is that whenever
from_cwrap<T>(self)is called, somehow inserting anything other than the instance as self (is that even possible?) would not be supported.Here are some of the less simple cases:
In the call to _rd_sum._fread_alloc,
data_filesis a local variable, so this is unproblematic. Similarly, the following have also always been unproblematic:_rd_sum._init_pandas_frame,keywords_rd_sum._init_pandas_frame_interp,keywords_rd_sum._init_pandas_frame_interp,time_points_rd_sum._dump_csv_line,key_words_rd_sum._export_csv,var_list_grid._fprintf_grdecl2,output_unit_grid._fprintf_GRID2,output_unit_grid._fprintf_EGRID2,output_unit_grid._compressed_kw_copy,kw_copy_grid._global_kw_copy,kw_copy_kw._fprintf_grdecl,file_grid._fwrite_grdecl,file_grid._fprintf_grdecl2,file_rd_sum._dump_csv_line,file_grid._equal,otherFor the following function, parameter combinations the following holds: In the call to the function, giving None or an invalid BaseCClass would result in UB. After this change all UB cases are
TypeError._rd_sum._resample,time_points_grid._grdecl_create,zcorn_grid._grdecl_create,coord_grid._fwrite_grdecl,kw_grid._load_column,kw_grid._load_column,column_grid._fprintf_grdecl2,kw_kw._load_grdecl,cfile_kw._load_grdecl,kwThe following would give a different exception if given None, but potentially a UB if a different BaseCClass. We have now introduced a
TypeErrorinstead._grid._compressed_kw_copy,kw_grid._global_kw_copy,kw_grid._export_data_as_int,kw_grid._export_data_as_double,kwFor
_grid._grdecl_create,actnumand_grid._grdecl_create,mapaxes, the only difference is that these could always be None. However, we have now introduced aTypeErrorif not the correct BaseCClass.