> +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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
