[FFmpeg-devel] [PATCH v4 2/3] avcodec/sanm: fobj left/top are signed

[Manuel Lauss]

Servus Michael,

On Mon, Mar 10, 2025 at 9:32 PM Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> Hi
>
> On Sun, Mar 09, 2025 at 04:52:25PM +0100, Manuel Lauss wrote:
> > Hi Michael,
> >
> > On Sat, Mar 8, 2025 at 8:11 PM Michael Niedermayer
> > <michael at niedermayer.cc> wrote:
> > >
> > > Hi Manuel
> > >
> > > On Tue, Mar 04, 2025 at 06:07:18PM +0100, Manuel Lauss wrote:
> > > > The left and top parameters of an FOBJ are signed values.
> > > >
> > > > Signed-off-by: Manuel Lauss <manuel.lauss at gmail.com>
> > > > ---
> > > > v4: revert v3, it arose due to a misunderstanding
> > > > v3: change the bytestream accessor to signed too
> > > > v2: no changes
> > > >  libavcodec/sanm.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
> > > > index a4f0a28c7c..71dbac4320 100644
> > > > --- a/libavcodec/sanm.c
> > > > +++ b/libavcodec/sanm.c
> > > > @@ -1238,8 +1238,8 @@ static int old_codec48(SANMVideoContext *ctx, int width, int height)
> > > >  static int process_frame_obj(SANMVideoContext *ctx)
> > > >  {
> > > >      uint16_t codec = bytestream2_get_le16u(&ctx->gb);
> > > > -    uint16_t left  = bytestream2_get_le16u(&ctx->gb);
> > > > -    uint16_t top   = bytestream2_get_le16u(&ctx->gb);
> > > > +    int16_t  left  = bytestream2_get_le16u(&ctx->gb);
> > > > +    int16_t  top   = bytestream2_get_le16u(&ctx->gb);
> > > >      uint16_t w     = bytestream2_get_le16u(&ctx->gb);
> > > >      uint16_t h     = bytestream2_get_le16u(&ctx->gb);
> > >
> > > Does the following code also handle all error conditions that
> > > negative left/top could now trigger ?
> >
> > For the LucasArts titles that sanm.c currently supports well,
> > no negative values are ever encountered.
> > I let ffplay run through maybe 1/3 of the Rebel Assault 1 videos,
> > which are the only ones that make use of negative values, but
> > didn't encounter any crashes; mostly because the codecs it
> > uses aren't supported by ffmpeg/sanm (yet).
>
> My concern is not that it crashes my concern is that manually craftet
> files could result in arbitrary code execution if theres any out of
> array accesses.
> Did you check that negative values are safe in that respect ?

I looked over the code and came up with something new.
Please disregard this and the 3/3 patch, I will send a more
comprehensive patchset for sanm shortly,
which hopefully also addresses your concern.

Thanks for the review!
      Manuel