Make struct initialization logic safe
Closed this issue · 0 comments
Currently the logic for creating the various context structs either leaves them uninitialized or unsafely zero-initializes their fields. The struct fields are then initialized field-by-field. This logic was correct in C, but can lead to undefined behavior in rust if we zero-initialize a type like Vec
or leave a field uninitialized before using the struct.
To solve this, we need to invert the initialization logic to only construct the final struct with valid values for all of the fields, i.e. rather than allocating the struct and then initializing the fields one-by-one, we create the value for the fields and then initialize the outer struct using Rust's normal struct initialization syntax. In some cases, we may just need a Default
impl so that we can safely default-initialize the struct before performing the rest of the initialization logic.
In order to identify places this is happening, look for any of the following operations:
- Calls to
mem::zeroed
orMaybeUninit::zeroed
. - Calls to
malloc
orrav1d_alloc_aligned
.
The following types/fields are being unsafely initialized:
-
Rav1dContext
- Allocated and initialized inrav1d_open
. -
Rav1dFrameData
- Allocated and initialized inrav1d_open
. -
TaskThreadData_delayed_fg
- Being zero-initialized inrav1d_open
. -
Rav1dFrameContext_lf
- Part ofRav1dContext
, initialized inrav1d_open
. -
Rav1dPicture
- Zero-initialized indav1d_get_picture
anddav1d_apply_grain
. -
Rav1dTaskContext
-new
method initializes fields withmem::zeroed
. Called fromrav1d_open
. -
Rav1dTileState
- Allocated inrav1d_decode_frame_init
. -
BlockContext
- Allocated inrav1d_decode_frame_init
. -
Rav1dFrameHeader
- Being partially zero-initialized inparse_frame_hdr
. -
refmvs_block
- Allocated inrav1d_refmvs_init_frame
. -
refmvs_temporal_block
- Allocated inrav1d_refmvs_init_frame
.
We have the following *_init
functions that take a mutable pointer to some struct and initialize its fields. These will have to likewise be "inverted" where possible to directly return the initialized struct instead of taking a pointer to a struct to initialize. In cases where the the initialization logic makes this impossible, we need to at least make sure that the struct being initialized with default values when it's first created.
-
rav1d_cdf_thread_init_static
-
rav1d_decode_frame_init
-
rav1d_decode_frame_init_cdf
-
setup_tile
(called fromrav1d_decode_frame_init_cdf
) -
rav1d_refmvs_init
-
rav1d_refmvs_init_frame
-
rav1d_refmvs_tile_sbrow_init
The following are functions for initializing function pointer structs. These are generally the leaf nodes of the initialization logic and should be straightforward to "invert". The DSP structs are currently being zero-initialized, which is UB for function pointers.
-
rav1d_cdef_dsp_init
-
rav1d_intra_pred_dsp_init
-
rav1d_itx_dsp_init
-
rav1d_loop_filter_dsp_init
-
rav1d_loop_restoration_dsp_init
-
rav1d_mc_dsp_init
-
rav1d_msac_init
-
rav1d_refmvs_dsp_init