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

[Michael Niedermayer]

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 ?

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250310/6649a277/attachment.sig>