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

From: Manuel Lauss <manuel.lauss@gmail.com>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH v4 2/3] avcodec/sanm: fobj left/top are signed
Date: Tue, 11 Mar 2025 13:59:48 +0100
Message-ID: <CAOLZvyGQyV=Pgs=ARud9f5GL3hLAu0snU+5PsAHwWHq=bt4RWw@mail.gmail.com> (raw)
In-Reply-To: <20250310203242.GO4991@pb2>

Servus Michael,

On Mon, Mar 10, 2025 at 9:32 PM Michael Niedermayer
<michael@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@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@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".