[FFmpeg-devel] [PATCH] Add FITS Demuxer

[Paras Chadha]

On Tue, Jul 4, 2017 at 4:12 AM, Reimar Döffinger <Reimar.Doeffinger at gmx.de>
wrote:

>
> > +static int64_t find_size(AVIOContext * pb, FITSContext * fits)
> > +{
> > +    int bitpix, naxis, dim_no, i, naxisn[999], groups=0;
> > +    int64_t header_size = 0, data_size=0, ret, pcount=0, gcount=1, d;
> > +    char buf[81], c;
>
> This is more than 4kB of data on the stack.
> Large stack arrays have a huge amount of security implications, please put
> such buffers (if really needed) into the context.
>
> > +    memset(buf, 0, 81);
> > +    if ((ret = avio_read(pb, buf, 80)) != 80)
> > +        return AVERROR_EOF;
>
> Seems a bit overkill to memset all if only one is not being read.
> Though mostly I wanted to comment that I still think it's really bad style
> to put the assignment into the if, it makes it quite a bit harder to read
> and we've had a few bugs because of it just to avoid one line of code...
>

Ok, i will move this assignment outside.


>
> > +    if (!strncmp(buf, "SIMPLE", 6) || !strncmp(buf, "XTENSION= 'IMAGE",
> 16)) {
> > +        fits->image = 1;
> > +    } else {
> > +        fits->image = 0;
> > +    }
>
> Could be simplified to fits->image = !strncmp...
>

Yes, will do this.


>
> > +    header_size = ceil(header_size/2880.0)*2880;
>
> Please avoid floating point. It rarely ends well to use it, especially
> since its range is smaller than the range of the int64 type you operate on.
> It's the same as doing ((header_size + 2879)/2880)*2880, though there
> might be nicer-looking ways.
>

ok, i will make this change.


>
> > +    if (header_size < 0)
> > +        return AVERROR_INVALIDDATA;
>
> As you said, this can only happen in case of overflow.
> But if it overflowed (though I would claim this is not really possible),
> you already invoked undefined behaviour. Checking after undefined behaviour
> happened is like putting a bandage on the broken hand of a beheaded
> person...
> Not to mention that it doesn't even catch the loss of precision in the
> floating-point operation above.
>

okay, i know it is really not possible. Just to be on safe side, i added
this check. Should i remove it ?


>
> > +            data_size *= naxisn[i];
> > +            if (data_size < 0)
> > +                return AVERROR_INVALIDDATA;
>
> If this is supposed to be overlow check as well: same issue as before: you
> need to PREVENT the overflow, afterwards it's too late, at least for signed
> types.
> Also the golden rule of input sanitization: sanitize/range check FIRST,
> THEN do the arithmetic.
> NEVER do it the other way run without at least spending 30 minutes per
> operation making sure it's really still safe.
>

Actually, here also overflow is not possible unless someone intentionally
put values of naxisn[i] that would cause overflow. Most of the FITS files
are in MB's or at max in GB's (although i have not seen any in GB). So, if
someone intentionally put wrong values of size in header then it would
crash or cause undefined behavior. Just to prevent that i have added a
check. So, can suggest a better way to do this ? I need the size of data to
allocate a packet and send it to decoder. Regarding range check, FITS
standard does not have any limit on the range of values of dimension,
theoretically it can be anything but practically ofcourse it cannot exceed
the limits of 64 bit integer. So, how should i go about this ?


>
> >
> > +    fits->image = 0;
> > +    pos = avio_tell(s->pb);
> > +    while (!fits->image) {
> > +        if ((ret = avio_seek(s->pb, pos+size, SEEK_SET)) < 0)
> > +            return ret;
> > +
> > +        if (avio_feof(s->pb))
> > +            return AVERROR_EOF;
> > +
> > +        pos = avio_tell(s->pb);
> > +        if ((size = find_size(s->pb, fits)) < 0)
> > +            return size;
> > +    }
> > +
> > +    if ((ret = avio_seek(s->pb, pos, SEEK_SET)) < 0)
> > +        return ret;
>
> Does this seek ever do anything?
>

Yes, it seeks back the pointer to point to the begining of the header.


> Either way, I do not like all this seeking, especially not when they all
> use SEEK_SET, it makes it rather hard to see if this all works when the
> input is being streamed or not...


> > +    ret = av_get_packet(s->pb, pkt, size);
> > +    if (ret != size) {
> > +        if (ret > 0) av_packet_unref(pkt);
> > +        return AVERROR(EIO);
> > +    }
>
> I don't know what the current rules are, but back when I was more active
> the rule was to always extract as much data as possible.
> So if you only get a partial packet, return that, and maybe the
> application can still do at least something with it.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>