[FFmpeg-devel] [PATCH 1/3] avformat/mov: Increase support for common encryption.

[Jacob Trimble]

On Mon, Jan 8, 2018 at 5:19 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> 2018-01-08 23:16 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
>>> You can't remove API just like that without a deprecation period.
>>> Add a new av_aes_ctr_set_full_iv() function, and either leave
>>> av_aes_ctr_set_iv() alone or deprecate it if it effectively becomes
>>> superfluous after adding the new function.
>>>
>>> Also, this patch needs to be split in three. One for the aes_crt
>>> changes, one for the encryption_info changes, and one for mov changes,
>>> with a minor libavutil version bump for the former two, and the
>>> corresponding APIChanges entry.
>>> Alternatively, you could also squash the new encryption_info API from
>>> this patch into the one that actually introduces the entire feature.
>>
>> Whoops, I thought that was internal-only.  Done and split into its own change.
>>
>> On Sat, Jan 6, 2018 at 7:30 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>> 2018-01-05 20:49 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
>>>
>>>> +        if (!frag_stream_info->encryption_index) {
>>>> +            frag_stream_info->encryption_index = av_mallocz(sizeof(MOVEncryptionIndex));
>>>
>>> sizeof(variable), please.
>
> Do you disagree?
> I have no strong opinion, but I thought it makes the code more robust...

I did it most other places, but forgot it here.  Done.

>
>>> [...]
>>>
>>>> +    sample_count = avio_rb32(pb);
>>>> +
>>>> +    encryption_index->encrypted_samples =
>>>> av_mallocz_array(sizeof(AVEncryptionInfo*), sample_count);
>>>
>>> This should be avoided if possible, see below.
>>>
>>>> +    if (!encryption_index->encrypted_samples) {
>>>>          return AVERROR(ENOMEM);
>>>>      }
>>>> +    encryption_index->nb_encrypted_samples = sample_count;
>>>>
>>>> -    return av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
>>>> +    for (i = 0; i < sample_count; i++) {
>>>
>>> Please check here for eof...
>>>
>>>> +        ret = mov_read_sample_encryption_info(c, pb, sc,
>>>> &encryption_index->encrypted_samples[i], use_subsamples);
>>>
>>> ... and insert a realloc here to avoid the large allocation above, see 1112ba01.
>>
>> Done.
>
>> +    if (use_subsamples) {
>> +        subsample_count = avio_rb16(pb);
>> +        for (i = 0; i < subsample_count && !pb->eof_reached; i++) {
>> +            unsigned int min_subsamples = FFMIN(FFMAX(i, 1024 * 1024), subsample_count);
>> +            subsamples = av_fast_realloc((*sample)->subsamples, &alloc_size,
>> +                                         min_subsamples * sizeof(*subsamples));
>
> The reason I did not comment on this block in the last mail is that
> iiuc, the maximum allocation here is INT16_MAX * 8 which seems
> acceptable (and there cannot be a realloc with the chosen initial
> limit).

Ok, changed back.

>
>> +    sample_count = avio_rb32(pb);
>
> You have to check here...
>
> +    for (i = 0; i < sample_count && !pb->eof_reached; i++) {
> +        unsigned int min_samples = FFMIN(FFMAX(i, 1024 * 1024), sample_count);
> +        encrypted_samples =
> av_fast_realloc(encryption_index->encrypted_samples, &alloc_size,
>
> +                                            min_samples *
> sizeof(*encrypted_samples));
>
> ... if this product could overflow for the final reallocation, see 1112ba01.

Done

>
> Thank you, Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avformat-mov-Increase-support-for-v3.patch
Type: text/x-patch
Size: 34648 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180109/eb642ad2/attachment.bin>