On Tue, Apr 12, 2011 at 04:34:19PM +0200, Max Horn wrote:
> Hi again!
>
> Am 12.04.2011 um 15:59 schrieb Kostya:
>
> > On Tue, Apr 12, 2011 at 03:35:04PM +0200, Max Horn wrote:
> >>
>
> [...]
>
> >>
> >> + /* parse fmt header */
> >> + tag = avio_rl32(pb);
> >> + if (tag != MKTAG('f', 'm', 't', ' '))
> >> + return -1;
> >> + size = avio_rl32(pb);
> >> + if (size < 0)
> >> + return -1;
> >
> > not possible since avio_rl32() will return unsigned value (and it will fit
> > into int64_t)
> Fine by me. Note that this part of the code is copied verbatim from wav.c, so
> if you think it's wrong here, you may want to "fix" this there, too.
It may happen there when it searches for next tag, not after reading size
directly.
> [...]
>
> >> + av_log(s, AV_LOG_WARNING, "dpds chunk size "PRId64" not
> >> divisible by 4\n", size);
> >> + }
> >> + dpds_table_size = size / 4;
> >
> > someone would complain about possible security hole if sizeof(uint32_t) > 4,
> > but maybe a general check for insanely large dpds_table_size values would be
> > good (like >= INT_MAX / 4 and also == 0).
>
> OK... though my code does catch the dpds_table_siz ==0 case later on, and a
> huge malloc should just fail (which is also handled)... Bu sure, I'll add
> those guards, won't hurt.
the main concern is arithmetical overflow so calculated table size is smaller
than indended so writing past its bounds may crash program (that's just a
general tip, not that it applies here)
And allocating 0 bytes is stupid IMO.
> I guess this means I should report similar and worse things in all the other
> parts of libav code? Like e.g. ff_get_wav_header, which is used by several
> demuxers, and which has no checks against invalid cbData, and does not check
> whether av_mallocz returns NULL? This looks like a much easier to exploit
> code spot to me... :)
It's a good place to remind our motto: "Patch is welcome"
(yes, not checking alloc result is bad)
> >>
> >> + if (dpds_table && dpds_table_size) {
> >> + int64_t cur_pos;
> >> + const uint32_t bytes_per_sample
> >> + = (st->codec->channels *
> >> st->codec->bits_per_coded_sample) >> 3;
> >> +
> >> + /* Estimate the duration from the total number of output bytes. */
> >> + const uint64_t total_decoded_bytes = dpds_table[dpds_table_size -
> >> 1];
> >> + st->duration = total_decoded_bytes / bytes_per_sample;
> >
> > why not st->duration = dpds_table_size
>
> Because dpds_table_size is the number of entries in the table (e.g. 20), and
> doesn't tell us anything about the actual duration ?
>
> But maybe I misunderstand the duration field (wouldn't surprise me, as the
> libav docs aren't very comprehensive *cough*) I interpreted "duration of the
> stream, in stream time base" as meaning "duration is measured in units
> corresponding to those defined by the stream's time_base". And the time_base
> in turn is set by a av_set_pts_info() in my code to the sample rate.
yes, st->duration should be number of samples in your case, my bad.
> <rant>
> Not that I understand why av_set_pts_info() sets the time base, its name /
> description didn't make that clear to me at all, I only discovered this by
> grepping around. Until a few minutes ago I also had no idea what a pts is,
> because I was only looking at libavformat. Thankfully the friendly people on
> IRC #libav-devel explained it. And then I also discovered that it is
> "defiend" in avcodec.h -- since I was only looking at libavformat, I didn't
> realize that. I really was thinking that the various libavFOO sublibs are
> more or less independent, and thus can be understood in isolation, but
> obviously this assumption was dead wrong. :/
the only goal of libavformat is to serve libavcodec, so you can understand the
latter without the former but not vice versa. And sometimes they both use
stuff from libavutil.
> And even now that I know what a PTS is, it seems slightly weird to me that I
> should specify a sample rate / time base as a "timestamp". Wouldn't it make
> more sense to rename av_set_pts_info to av_set_timebase ?
> As it is, I really had fun appreciating the subtle difference between "the
> pts" (=time base), and "a pts" (e.g. start_time is a pts, but not the pts).
> Gee... :)
who said that developers are good at naming?
> </rant>
>
> OK, enough unconstructive ranting :). I know how it is with software growing
> over years and stuff. So back to more useful things: Attached is a revised
> patch hopefully addressing Kostya's concerns. Thank you very much for your
> feedback!
I remind you that there's also Diego who can complain about wrongly worded
comments, formatting style and such, but in general patch looks good for me.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
