[FFmpeg-devel] [PATCH v1 2/3] avcodec/jpeg2000dec: add support for HTJ2K block decoding

[Michael Niedermayer]

On Mon, Apr 03, 2023 at 07:20:05PM +0300, Caleb Etemesi wrote:
> Hi
> 
> On Mon, Apr 3, 2023 at 5:28 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Sun, Apr 02, 2023 at 03:21:08PM -0700, Pierre-Anthony Lemieux wrote:
> > > On Sun, Apr 2, 2023 at 3:17 PM Michael Niedermayer
> > > <michael at niedermayer.cc> wrote:
> > > >
> > > > On Fri, Mar 31, 2023 at 08:29:40AM -0700, pal at sandflow.com wrote:
> > > > > From: caleb <etemesicaleb at gmail.com>
> > [...]
> > > > [...]
> > > > > +/**
> > > > > + * Drops bits from lower bits in the bit buffer. buf contains the
> > bit buffers.
> > > > > + * nbits is the number of bits to remove.
> > > > > + */
> > > > > +av_always_inline
> > > > > +static void jpeg2000_bitbuf_drop_bits_lsb(StateVars *buf, uint8_t
> > nbits)
> > > > > +{
> > > > > +    if (buf->bits_left < nbits) {
> > > >
> > > > > +        av_log(NULL, AV_LOG_ERROR, "Invalid bit read of %d, bits in
> > buffer are %d\n", nbits, buf->bits_left);
> > > > > +        av_assert0(0);
> >
> 
> This one is present to ensure for all test cases, we never overread bytes

But can a crafted input stream trigger an assert failure ?

If no then the assert is ok

If yes then its not ok, i mean a
"./ffmpeg -i /dev/random" 
should not crash, it should provide the user with some error and teh code should
cleanly return not abort

I simply do not know if theres a check or something somewhere that ensures
this is ok and just a extra check (which is ok)


> 
> 
> > > >
> > > >
> > > > [...]
> > > > > +int
> > > > > +ff_jpeg2000_decode_htj2k(const Jpeg2000DecoderContext *s,
> > Jpeg2000CodingStyle *codsty, Jpeg2000T1Context *t1, Jpeg2000Cblk *cblk,
> > > > > +                         int width, int height, int magp, uint8_t
> > roi_shift)
> > > > > +{
> > > > > +    uint8_t p0 = 0;             // Number of placeholder passes
> > > > > +    uint32_t Lcup;              // Length of HT cleanup segment
> > > > > +    uint32_t Lref;              // Length of Refinement segment
> > > > > +    uint32_t Scup;              // HT cleanup segment suffix length
> > > > > +    uint32_t Pcup;              // HT cleanup segment prefix length
> > > > > +
> > > > > +    uint8_t S_blk;              // Number of skipped magnitude
> > bitplanes
> > > > > +    uint8_t pLSB;
> > > > > +
> > > > > +    uint8_t *Dcup;              // Byte of an HT cleanup segment
> > > > > +    uint8_t *Dref;              // Byte of an HT refinement segment
> > > > > +
> > > > > +    int z_blk;                  // Number of ht coding pass
> > > > > +
> > > > > +    uint8_t empty_passes;
> > > > > +
> > > > > +    StateVars mag_sgn;          // Magnitude and Sign
> > > > > +    StateVars mel;              // Adaptive run-length coding
> > > > > +    StateVars vlc;              // Variable Length coding
> > > > > +    StateVars sig_prop;         // Significance propagation
> > > > > +
> > > > > +    MelDecoderState mel_state;
> > > > > +
> > > > > +    int ret;
> > > > > +
> > > > > +    /* Temporary buffers */
> > > > > +    int32_t *sample_buf;
> > > > > +    uint8_t *block_states;
> > > > > +
> > > > > +    int32_t n, val;             // Post-processing
> > > > > +
> > > > > +    int32_t M_b = magp;
> > > > > +    av_assert0(width <= 1024U && height <= 1024U);
> > > > > +    av_assert0(width * height <= 4096);
> > > > > +    av_assert0(width * height > 0);
> > > >
> > > > Has this decoder been tested with some fuzzer ?
> > >
> > > Yes.
> >
> > ok
> >
> >
> > >
> > > > I see a bunch of asserts in it and i dont immedeatly see what would
> > prevent them from
> > > > triggering
> >
> > > maybe you can add comments to the asserts that explain why these are
> > > guranteed to be true thx
> >
> >
> These ones  are to check code block constraints, the jpeg2000 spec limits
> code blocks to be between 2^2 and 2^10.

The spec isnt that important here. The important part is that our code checks
this, so no input can trigger the asserts.

I think we do check this, so i think the asserts are ok

thx

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20230404/45814777/attachment.sig>