[FFmpeg-devel] [PATCH] Add FITS Demuxer

[Reimar Döffinger]

> +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...

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

Could be simplified to fits->image = !strncmp...

> +    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.

> +    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.

> +            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.

> 
> +    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?
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.