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.
[...]
>> + 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.
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... :)
>>
>> + 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.
<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. :/
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... :)
</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!
Cheers,
Max
0001-add-xWMA-demuxer.patch
Description: Binary data
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
