| Message ID | 20210408091734.7797-1-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavu/video_enc_params: make sure blocks are properly aligned | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Anton Khirnov: > --- > libavutil/video_enc_params.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c > index 635176ab91..1779a8799a 100644 > --- a/libavutil/video_enc_params.c > +++ b/libavutil/video_enc_params.c > @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, > unsigned int nb_blocks, size_t *out_size) > { > AVVideoEncParams *par; > - size_t size; > + size_t blocks_offset = FFALIGN(sizeof(*par), 8); > + size_t size = blocks_offset; > > - size = sizeof(*par); > if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams)) > return NULL; > size += sizeof(AVVideoBlockParams) * nb_blocks; > @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, > par->type = type; > par->nb_blocks = nb_blocks; > par->block_size = sizeof(AVVideoBlockParams); > - par->blocks_offset = sizeof(*par); > + par->blocks_offset = blocks_offset; > > if (out_size) > *out_size = size; > Wouldn't it be safer to just define a struct { AVVideoEncParams header; AVVideoBlockParams blocks[1]; } and use the offset of blocks in that? - Andreas
Quoting Andreas Rheinhardt (2021-04-08 11:41:41) > Anton Khirnov: > > --- > > libavutil/video_enc_params.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c > > index 635176ab91..1779a8799a 100644 > > --- a/libavutil/video_enc_params.c > > +++ b/libavutil/video_enc_params.c > > @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, > > unsigned int nb_blocks, size_t *out_size) > > { > > AVVideoEncParams *par; > > - size_t size; > > + size_t blocks_offset = FFALIGN(sizeof(*par), 8); > > + size_t size = blocks_offset; > > > > - size = sizeof(*par); > > if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams)) > > return NULL; > > size += sizeof(AVVideoBlockParams) * nb_blocks; > > @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, > > par->type = type; > > par->nb_blocks = nb_blocks; > > par->block_size = sizeof(AVVideoBlockParams); > > - par->blocks_offset = sizeof(*par); > > + par->blocks_offset = blocks_offset; > > > > if (out_size) > > *out_size = size; > > > Wouldn't it be safer to just define a struct { AVVideoEncParams header; > AVVideoBlockParams blocks[1]; } and use the offset of blocks in that? I don't see how it would be safer and it prevents you from adding new fields to AVVideoEncParams
Anton Khirnov: > Quoting Andreas Rheinhardt (2021-04-08 11:41:41) >> Anton Khirnov: >>> --- >>> libavutil/video_enc_params.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c >>> index 635176ab91..1779a8799a 100644 >>> --- a/libavutil/video_enc_params.c >>> +++ b/libavutil/video_enc_params.c >>> @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, >>> unsigned int nb_blocks, size_t *out_size) >>> { >>> AVVideoEncParams *par; >>> - size_t size; >>> + size_t blocks_offset = FFALIGN(sizeof(*par), 8); >>> + size_t size = blocks_offset; >>> >>> - size = sizeof(*par); >>> if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams)) >>> return NULL; >>> size += sizeof(AVVideoBlockParams) * nb_blocks; >>> @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, >>> par->type = type; >>> par->nb_blocks = nb_blocks; >>> par->block_size = sizeof(AVVideoBlockParams); >>> - par->blocks_offset = sizeof(*par); >>> + par->blocks_offset = blocks_offset; >>> >>> if (out_size) >>> *out_size = size; >>> >> Wouldn't it be safer to just define a struct { AVVideoEncParams header; >> AVVideoBlockParams blocks[1]; } and use the offset of blocks in that? > > I don't see how it would be safer and it prevents you from adding new > fields to AVVideoEncParams > Safer: You are making assumptions about the alignment of AVVideoBlockParams; these assumptions may or may not be fulfilled in practice. And I really don't get your point about this preventing the addition of new fields to AVVideoEncParams. If something is added to AVVideoEncParams, sizeof(header) and offsetof(blocks) will grow automatically as required. - Andreas
Quoting Andreas Rheinhardt (2021-04-08 12:27:03) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2021-04-08 11:41:41) > >> Anton Khirnov: > >>> --- > >>> libavutil/video_enc_params.c | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c > >>> index 635176ab91..1779a8799a 100644 > >>> --- a/libavutil/video_enc_params.c > >>> +++ b/libavutil/video_enc_params.c > >>> @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, > >>> unsigned int nb_blocks, size_t *out_size) > >>> { > >>> AVVideoEncParams *par; > >>> - size_t size; > >>> + size_t blocks_offset = FFALIGN(sizeof(*par), 8); > >>> + size_t size = blocks_offset; > >>> > >>> - size = sizeof(*par); > >>> if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams)) > >>> return NULL; > >>> size += sizeof(AVVideoBlockParams) * nb_blocks; > >>> @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, > >>> par->type = type; > >>> par->nb_blocks = nb_blocks; > >>> par->block_size = sizeof(AVVideoBlockParams); > >>> - par->blocks_offset = sizeof(*par); > >>> + par->blocks_offset = blocks_offset; > >>> > >>> if (out_size) > >>> *out_size = size; > >>> > >> Wouldn't it be safer to just define a struct { AVVideoEncParams header; > >> AVVideoBlockParams blocks[1]; } and use the offset of blocks in that? > > > > I don't see how it would be safer and it prevents you from adding new > > fields to AVVideoEncParams > > > Safer: You are making assumptions about the alignment of > AVVideoBlockParams; these assumptions may or may not be fulfilled in > practice. > And I really don't get your point about this preventing the addition of > new fields to AVVideoEncParams. If something is added to > AVVideoEncParams, sizeof(header) and offsetof(blocks) will grow > automatically as required. Nevermind, I misunderstood your suggestion. I suppose it works better indeed.
diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c index 635176ab91..1779a8799a 100644 --- a/libavutil/video_enc_params.c +++ b/libavutil/video_enc_params.c @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, unsigned int nb_blocks, size_t *out_size) { AVVideoEncParams *par; - size_t size; + size_t blocks_offset = FFALIGN(sizeof(*par), 8); + size_t size = blocks_offset; - size = sizeof(*par); if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams)) return NULL; size += sizeof(AVVideoBlockParams) * nb_blocks; @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type, par->type = type; par->nb_blocks = nb_blocks; par->block_size = sizeof(AVVideoBlockParams); - par->blocks_offset = sizeof(*par); + par->blocks_offset = blocks_offset; if (out_size) *out_size = size;