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

[Carl Eugen Hoyos]

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

>> [...]
>>
>>> +    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).

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

Thank you, Carl Eugen