Add UNCONST profile extensions and rework the tile info API - #236
Conversation
|
LGTM |
Signed-off-by: KP Choi <kp5.choi@samsung.com>
Signed-off-by: KP Choi <kp5.choi@samsung.com>
Signed-off-by: KP Choi <kp5.choi@samsung.com>
Signed-off-by: KP Choi <kp5.choi@samsung.com>
Signed-off-by: KP Choi <kp5.choi@samsung.com>
Signed-off-by: KP Choi <kp5.choi@samsung.com>
Signed-off-by: KP Choi <kp5.choi@samsung.com>
Signed-off-by: KP Choi <kp5.choi@samsung.com>
jsperrier1
left a comment
There was a problem hiding this comment.
Review notes from the perspective of a downstream consumer of the tile-info and allocator changes. Three code issues are marked inline; one compatibility question follows.
Inline findings
- Heap buffer overflow from integer truncation in the decoder tile allocation (
dec_frm_prepare).oapv_ops_mem.malloctakes anunsigned intsize, while the call site computes a 64-bitsize_t, so the size is silently truncated; theoapv_mseton the following line uses the full untruncated value. Reachable from a crafted bitstream. Details inline. tile_capis not reset when the allocation fails, leavingctx->tile == NULLwith a stale non-zero capacity, which leads to a NULL dereference on a later call. The same pattern occurs in both encoder and decoder.- An unvalidated
intmultiply inenc_frm_prepare. It does not appear exploitable, since the topology validation rejects oversized configurations immediately afterwards; noted for consistency with thes64handling already used inoapv_validate_tile_topology.
Compatibility question: frame-header tile sizes are now discarded
In dec_vlc_tile_info the per-tile sizes are read and dropped:
u32 tile_size = oapv_bsr_read(bs, 32); // parsed for validation onlyand fh.tile_size[] is removed from oapv_fh_t. This removes the only means of obtaining per-tile sizes without decoding, which is the purpose of tile_size_present_in_fh_flag, and is what the selective/multi-mip decoding in #228 uses to compute per-tile offsets for random access.
Would retaining those parsed sizes, or surfacing them through oapvd_info_tile(), be acceptable? Without them the tiled/mip random-access path cannot be built on this change, and the flag has no consumer. A follow-up patch against this branch can be provided if that is preferable to expanding the scope of this PR.
| // allocate tile array to fit this frame's tile partitioning | ||
| if(ctx->num_tiles > ctx->tile_cap) { | ||
| oapv_ops_free(ctx, ctx->tile); | ||
| ctx->tile = (oapvd_tile_t *)oapv_ops_malloc(ctx, sizeof(oapvd_tile_t) * ctx->num_tiles); |
There was a problem hiding this comment.
Heap buffer overflow via integer truncation.
oapv_ops_mem.malloc is declared with a 32-bit size:
void *(*malloc)(void *udata, unsigned int size); // inc/oapv.hHere sizeof(oapvd_tile_t) * ctx->num_tiles is computed as a 64-bit size_t and is implicitly truncated to unsigned int at the call, while oapv_mset two lines below uses the full, untruncated size. The buffer is therefore under-allocated and then immediately memset past its end.
Reachability (sizeof(oapvd_tile_t) is 88):
- the product exceeds 2^32 once
num_tiles >= 48,806,447; - with an UNCONST profile at the 1-MB minimum tile size and
frame_widthnear itsu(24)maximum (~16.7M),frame_heightneed only be >= 745 px; oapv_validate_tile_topology()accepts this, as 48.8M is well underINT_MAX.
Since tile_size_present_in_fh_flag may be 0, no per-tile payload is required: a frame header of a few dozen bytes is sufficient to request a ~4.3 GB allocation that truncates. This is a significant amplification factor in addition to the memory-safety issue, on decoder-side attacker-controlled input, i.e. the same class as the fuzz hardening in #224/#226.
Minimum fix, validating the byte size before allocating:
s64 tile_bytes = (s64)sizeof(oapvd_tile_t) * ctx->num_tiles;
oapv_assert_rv(tile_bytes <= (s64)UINT_MAX, OAPV_ERR_MALFORMED_BITSTREAM);Bounding num_tiles against the frame area, and ideally against the available input size, rather than only against INT_MAX, would additionally prevent a small header from driving a multi-gigabyte allocation. Widening the oapv_ops_mem sizes to size_t would be a more durable fix, at the cost of an API change.
The encoder has the same truncation shape in enc_frm_prepare at line 1001.
| ctx->tile = (oapvd_tile_t *)oapv_ops_malloc(ctx, sizeof(oapvd_tile_t) * ctx->num_tiles); | ||
| oapv_assert_rv(ctx->tile != NULL, OAPV_ERR_OUT_OF_MEMORY); | ||
| oapv_mset(ctx->tile, 0, sizeof(oapvd_tile_t) * ctx->num_tiles); | ||
| ctx->tile_cap = ctx->num_tiles; |
There was a problem hiding this comment.
tile_cap is not reset if the allocation fails.
If oapv_ops_malloc returns NULL, oapv_assert_rv returns early and this line never executes, so ctx->tile is NULL while ctx->tile_cap retains the previous, non-zero capacity. The old buffer has already been freed above.
On a subsequent call with num_tiles <= tile_cap, the entire if block is skipped and ctx->tile is dereferenced while NULL (in dec_set_tile_info, then in the ctx->tile[i].bs_beg loop), producing a crash rather than the recoverable OAPV_ERR_OUT_OF_MEMORY the caller would otherwise receive.
Clearing the capacity alongside the pointer resolves this:
if(ctx->num_tiles > ctx->tile_cap) {
oapv_ops_free(ctx, ctx->tile);
ctx->tile = NULL;
ctx->tile_cap = 0; // keep pointer and capacity consistent on failure
ctx->tile = (oapvd_tile_t *)oapv_ops_malloc(ctx, ...);
oapv_assert_rv(ctx->tile != NULL, OAPV_ERR_OUT_OF_MEMORY);
...
ctx->tile_cap = ctx->num_tiles;
}The identical pattern occurs in the encoder at line 1004.
| ctx->fn_imgb_pad(imgb_i, ctx->w, ctx->h, ctx->c_sft); | ||
|
|
||
| // allocate tile array to fit this frame's tile partitioning | ||
| int num_tiles = oapv_div_round_up(ctx->w, param->tile_w) * oapv_div_round_up(ctx->h, param->tile_h); |
There was a problem hiding this comment.
This int multiply can overflow before it is validated.
Both operands are int, so for an UNCONST profile with small tiles and a large frame the product can overflow. Signed overflow is undefined behaviour, and a wrapped-negative or small-positive result then drives the allocation decision on the following line.
enc_set_tile_info() -> oapv_validate_tile_topology() rejects oversized configurations immediately afterwards, so this does not appear to be exploitable. It is however inconsistent with the s64 handling already present inside oapv_validate_tile_topology, and the allocation size is computed before that validation runs. Computing in s64, or validating the topology before allocating, would close the gap:
s64 num_tiles = (s64)oapv_div_round_up(ctx->w, param->tile_w) *
oapv_div_round_up(ctx->h, param->tile_h);Signed-off-by: KP Choi <kp5.choi@samsung.com> # Conflicts: # app/oapv_app_dec.c # src/oapv.c
This implements the 16K small-tile support proposed by @jsperrier1 in #228 as a set of OpenAPV profile extensions, together with the API changes needed to handle large tile counts. The implementation was written with reference to the code changes in #228. See docs/profile_ext.md for the extension definition.
UNCONST profiles
<profile>-UNCONSTextensions for all seven RFC 9924 profiles (e.g.422-10-UNCONST,profile_idc43), defined in a new document docs/profile_ext.md together with the numbering convention and the existing 16C12 profilesOAPV_MAX_TILESarrays, so the tile count is only bounded by the frame sizeTile info API rework (source-incompatible changes)
oapv_frm_info_tcarries the tile partitioning of a frameoapvd_info_tile()fills caller-provided tile positions with a capacity check; the fixed 400-entryoapv_tile_info_tis removedoapvd_decode_frame()takes a tile index list for partial decoding, and--cyclic-tile-decodingtakes the number of tiles to decode per frameFixes
OAPV_CFG_SET_DISABLE_COMPANDINGwas overwritten by the per-frame companding derivation; it is now kept separately and overrides itDocumentation