memorysafety/rav1d

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 or MaybeUninit::zeroed.
  • Calls to malloc or rav1d_alloc_aligned.

The following types/fields are being unsafely initialized:

  • Rav1dContext - Allocated and initialized in rav1d_open.
  • Rav1dFrameData - Allocated and initialized in rav1d_open.
  • TaskThreadData_delayed_fg - Being zero-initialized in rav1d_open.
  • Rav1dFrameContext_lf - Part of Rav1dContext, initialized in rav1d_open.
  • Rav1dPicture - Zero-initialized in dav1d_get_picture and dav1d_apply_grain.
  • Rav1dTaskContext - new method initializes fields with mem::zeroed. Called from rav1d_open.
  • Rav1dTileState - Allocated in rav1d_decode_frame_init.
  • BlockContext - Allocated in rav1d_decode_frame_init.
  • Rav1dFrameHeader - Being partially zero-initialized in parse_frame_hdr.
  • refmvs_block - Allocated in rav1d_refmvs_init_frame.
  • refmvs_temporal_block - Allocated in rav1d_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 from rav1d_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