| Message ID | CAO7y9i_a22pze9NXJtxQLhy9o09ovWT3oTF3wAaNNvKkB52=SA@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 07, 2018 at 04:59:33PM -0700, Jacob Trimble wrote: > On Mon, May 7, 2018 at 3:18 PM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Mon, Apr 23, 2018 at 11:03:57AM -0700, Jacob Trimble wrote: > >> While integrating my encryption info changes, I noticed a problem with > >> the init info structs. I implemented them as side-data on the Stream. > >> But this means there can only be one per stream. However, there can > >> be multiple 'pssh' atoms in a single stream (e.g. for key rotation or > >> multiple key systems). (sorry for not noticing sooner) > >> > >> Attached is a patch to fix this by making the init info a > >> singly-linked-list. It is ABI compatible and is still easy to use and > >> understand. The alternative would be to break ABI and have the > >> side-data methods return an array of pointers. I could do that > >> instead if that is preferable. > > > >> encryption_info.c | 154 +++++++++++++++++++++++++++++++++++------------------- > >> encryption_info.h | 5 + > >> 2 files changed, 106 insertions(+), 53 deletions(-) > >> e5eecc73a6997bbd11541771372d38ea9c3972a7 0001-libavutil-encryption_info-Allow-multiple-init-info.patch > >> From bb941a77e882e93629d63d63059d0063b9519e29 Mon Sep 17 00:00:00 2001 > >> From: Jacob Trimble <modmaker@google.com> > >> Date: Mon, 23 Apr 2018 10:33:58 -0700 > >> Subject: [PATCH] libavutil/encryption_info: Allow multiple init info. > >> > >> It is possible for there to be multiple encryption init info structure. > >> For example, to support multiple key systems or in key rotation. This > >> changes the AVEncryptionInitInfo struct to be a linked list so there > >> can be multiple structs without breaking ABI. > >> > >> Signed-off-by: Jacob Trimble <modmaker@google.com> > >> --- > >> libavutil/encryption_info.c | 154 +++++++++++++++++++++++------------- > >> libavutil/encryption_info.h | 5 ++ > >> 2 files changed, 106 insertions(+), 53 deletions(-) > >> > >> diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c > >> index 20a752d6b4..9935c10d74 100644 > >> --- a/libavutil/encryption_info.c > >> +++ b/libavutil/encryption_info.c > >> @@ -160,13 +160,16 @@ uint8_t *av_encryption_info_add_side_data(const AVEncryptionInfo *info, size_t * > >> } > >> > >> // The format of the AVEncryptionInitInfo side data: > >> -// u32be system_id_size > >> -// u32be num_key_ids > >> -// u32be key_id_size > >> -// u32be data_size > >> -// u8[system_id_size] system_id > >> -// u8[key_id_size][num_key_id] key_ids > >> -// u8[data_size] data > >> +// u32be init_info_count > >> +// { > >> +// u32be system_id_size > >> +// u32be num_key_ids > >> +// u32be key_id_size > >> +// u32be data_size > >> +// u8[system_id_size] system_id > >> +// u8[key_id_size][num_key_id] key_ids > >> +// u8[data_size] data > >> +// }[init_info_count] > >> > >> #define FF_ENCRYPTION_INIT_INFO_EXTRA 16 > >> > >> @@ -215,6 +218,7 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) > >> for (i = 0; i < info->num_key_ids; i++) { > >> av_free(info->key_ids[i]); > >> } > >> + av_encryption_init_info_free(info->next); > >> av_free(info->system_id); > >> av_free(info->key_ids); > >> av_free(info->data); > >> @@ -225,71 +229,115 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) > >> AVEncryptionInitInfo *av_encryption_init_info_get_side_data( > >> const uint8_t *side_data, size_t side_data_size) > >> { > >> - AVEncryptionInitInfo *info; > >> + AVEncryptionInitInfo *ret = NULL, *info; > >> uint64_t system_id_size, num_key_ids, key_id_size, data_size, i; > >> + uint64_t init_info_count; > >> > >> - if (!side_data || side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA) > >> - return NULL; > >> - > >> - system_id_size = AV_RB32(side_data); > > [...] > >> + init_info_count = AV_RB32(side_data); > > > > i may be missing something but this looks like the meaning of the first > > field changes, this thus doesnt look compatible to me > > It changes the binary format of the side-data, but that was explicitly > not part of ABI. The fields in the structs are unchanged. This would > only cause a problem if the side data bytes were stored out-of-band > from a different version of FFmpeg. yes, right. its side data that is neighter a C struct nor a well defined byte stream its a opaque block that can only be passed to libavutil which then translates it into a C struct. its not new but it still feels clumsy to use this as means to pass data around [...]
On Tue, May 8, 2018 at 3:47 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, May 07, 2018 at 04:59:33PM -0700, Jacob Trimble wrote: >> On Mon, May 7, 2018 at 3:18 PM, Michael Niedermayer >> <michael@niedermayer.cc> wrote: >> > On Mon, Apr 23, 2018 at 11:03:57AM -0700, Jacob Trimble wrote: >> >> While integrating my encryption info changes, I noticed a problem with >> >> the init info structs. I implemented them as side-data on the Stream. >> >> But this means there can only be one per stream. However, there can >> >> be multiple 'pssh' atoms in a single stream (e.g. for key rotation or >> >> multiple key systems). (sorry for not noticing sooner) >> >> >> >> Attached is a patch to fix this by making the init info a >> >> singly-linked-list. It is ABI compatible and is still easy to use and >> >> understand. The alternative would be to break ABI and have the >> >> side-data methods return an array of pointers. I could do that >> >> instead if that is preferable. >> > >> >> encryption_info.c | 154 +++++++++++++++++++++++++++++++++++------------------- >> >> encryption_info.h | 5 + >> >> 2 files changed, 106 insertions(+), 53 deletions(-) >> >> e5eecc73a6997bbd11541771372d38ea9c3972a7 0001-libavutil-encryption_info-Allow-multiple-init-info.patch >> >> From bb941a77e882e93629d63d63059d0063b9519e29 Mon Sep 17 00:00:00 2001 >> >> From: Jacob Trimble <modmaker@google.com> >> >> Date: Mon, 23 Apr 2018 10:33:58 -0700 >> >> Subject: [PATCH] libavutil/encryption_info: Allow multiple init info. >> >> >> >> It is possible for there to be multiple encryption init info structure. >> >> For example, to support multiple key systems or in key rotation. This >> >> changes the AVEncryptionInitInfo struct to be a linked list so there >> >> can be multiple structs without breaking ABI. >> >> >> >> Signed-off-by: Jacob Trimble <modmaker@google.com> >> >> --- >> >> libavutil/encryption_info.c | 154 +++++++++++++++++++++++------------- >> >> libavutil/encryption_info.h | 5 ++ >> >> 2 files changed, 106 insertions(+), 53 deletions(-) >> >> >> >> diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c >> >> index 20a752d6b4..9935c10d74 100644 >> >> --- a/libavutil/encryption_info.c >> >> +++ b/libavutil/encryption_info.c >> >> @@ -160,13 +160,16 @@ uint8_t *av_encryption_info_add_side_data(const AVEncryptionInfo *info, size_t * >> >> } >> >> >> >> // The format of the AVEncryptionInitInfo side data: >> >> -// u32be system_id_size >> >> -// u32be num_key_ids >> >> -// u32be key_id_size >> >> -// u32be data_size >> >> -// u8[system_id_size] system_id >> >> -// u8[key_id_size][num_key_id] key_ids >> >> -// u8[data_size] data >> >> +// u32be init_info_count >> >> +// { >> >> +// u32be system_id_size >> >> +// u32be num_key_ids >> >> +// u32be key_id_size >> >> +// u32be data_size >> >> +// u8[system_id_size] system_id >> >> +// u8[key_id_size][num_key_id] key_ids >> >> +// u8[data_size] data >> >> +// }[init_info_count] >> >> >> >> #define FF_ENCRYPTION_INIT_INFO_EXTRA 16 >> >> >> >> @@ -215,6 +218,7 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) >> >> for (i = 0; i < info->num_key_ids; i++) { >> >> av_free(info->key_ids[i]); >> >> } >> >> + av_encryption_init_info_free(info->next); >> >> av_free(info->system_id); >> >> av_free(info->key_ids); >> >> av_free(info->data); >> >> @@ -225,71 +229,115 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) >> >> AVEncryptionInitInfo *av_encryption_init_info_get_side_data( >> >> const uint8_t *side_data, size_t side_data_size) >> >> { >> >> - AVEncryptionInitInfo *info; >> >> + AVEncryptionInitInfo *ret = NULL, *info; >> >> uint64_t system_id_size, num_key_ids, key_id_size, data_size, i; >> >> + uint64_t init_info_count; >> >> >> >> - if (!side_data || side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA) >> >> - return NULL; >> >> - >> >> - system_id_size = AV_RB32(side_data); >> > [...] >> >> + init_info_count = AV_RB32(side_data); >> > >> > i may be missing something but this looks like the meaning of the first >> > field changes, this thus doesnt look compatible to me >> >> It changes the binary format of the side-data, but that was explicitly >> not part of ABI. The fields in the structs are unchanged. This would >> only cause a problem if the side data bytes were stored out-of-band >> from a different version of FFmpeg. > > yes, right. > its side data that is neighter a C struct nor a well defined byte stream > its a opaque block that can only be passed to libavutil which then translates > it into a C struct. > its not new but it still feels clumsy to use this as means to pass data around > I know it's weird, but this was the design that was decided on when this was added. Are there any changes you want for this patch? > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you think the mosad wants you dead since a long time then you are either > wrong or dead since a long time. > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, May 14, 2018 at 4:49 PM, Jacob Trimble <modmaker@google.com> wrote: > On Tue, May 8, 2018 at 3:47 PM, Michael Niedermayer > <michael@niedermayer.cc> wrote: >> On Mon, May 07, 2018 at 04:59:33PM -0700, Jacob Trimble wrote: >>> On Mon, May 7, 2018 at 3:18 PM, Michael Niedermayer >>> <michael@niedermayer.cc> wrote: >>> > On Mon, Apr 23, 2018 at 11:03:57AM -0700, Jacob Trimble wrote: >>> >> While integrating my encryption info changes, I noticed a problem with >>> >> the init info structs. I implemented them as side-data on the Stream. >>> >> But this means there can only be one per stream. However, there can >>> >> be multiple 'pssh' atoms in a single stream (e.g. for key rotation or >>> >> multiple key systems). (sorry for not noticing sooner) >>> >> >>> >> Attached is a patch to fix this by making the init info a >>> >> singly-linked-list. It is ABI compatible and is still easy to use and >>> >> understand. The alternative would be to break ABI and have the >>> >> side-data methods return an array of pointers. I could do that >>> >> instead if that is preferable. >>> > >>> >> encryption_info.c | 154 +++++++++++++++++++++++++++++++++++------------------- >>> >> encryption_info.h | 5 + >>> >> 2 files changed, 106 insertions(+), 53 deletions(-) >>> >> e5eecc73a6997bbd11541771372d38ea9c3972a7 0001-libavutil-encryption_info-Allow-multiple-init-info.patch >>> >> From bb941a77e882e93629d63d63059d0063b9519e29 Mon Sep 17 00:00:00 2001 >>> >> From: Jacob Trimble <modmaker@google.com> >>> >> Date: Mon, 23 Apr 2018 10:33:58 -0700 >>> >> Subject: [PATCH] libavutil/encryption_info: Allow multiple init info. >>> >> >>> >> It is possible for there to be multiple encryption init info structure. >>> >> For example, to support multiple key systems or in key rotation. This >>> >> changes the AVEncryptionInitInfo struct to be a linked list so there >>> >> can be multiple structs without breaking ABI. >>> >> >>> >> Signed-off-by: Jacob Trimble <modmaker@google.com> >>> >> --- >>> >> libavutil/encryption_info.c | 154 +++++++++++++++++++++++------------- >>> >> libavutil/encryption_info.h | 5 ++ >>> >> 2 files changed, 106 insertions(+), 53 deletions(-) >>> >> >>> >> diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c >>> >> index 20a752d6b4..9935c10d74 100644 >>> >> --- a/libavutil/encryption_info.c >>> >> +++ b/libavutil/encryption_info.c >>> >> @@ -160,13 +160,16 @@ uint8_t *av_encryption_info_add_side_data(const AVEncryptionInfo *info, size_t * >>> >> } >>> >> >>> >> // The format of the AVEncryptionInitInfo side data: >>> >> -// u32be system_id_size >>> >> -// u32be num_key_ids >>> >> -// u32be key_id_size >>> >> -// u32be data_size >>> >> -// u8[system_id_size] system_id >>> >> -// u8[key_id_size][num_key_id] key_ids >>> >> -// u8[data_size] data >>> >> +// u32be init_info_count >>> >> +// { >>> >> +// u32be system_id_size >>> >> +// u32be num_key_ids >>> >> +// u32be key_id_size >>> >> +// u32be data_size >>> >> +// u8[system_id_size] system_id >>> >> +// u8[key_id_size][num_key_id] key_ids >>> >> +// u8[data_size] data >>> >> +// }[init_info_count] >>> >> >>> >> #define FF_ENCRYPTION_INIT_INFO_EXTRA 16 >>> >> >>> >> @@ -215,6 +218,7 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) >>> >> for (i = 0; i < info->num_key_ids; i++) { >>> >> av_free(info->key_ids[i]); >>> >> } >>> >> + av_encryption_init_info_free(info->next); >>> >> av_free(info->system_id); >>> >> av_free(info->key_ids); >>> >> av_free(info->data); >>> >> @@ -225,71 +229,115 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) >>> >> AVEncryptionInitInfo *av_encryption_init_info_get_side_data( >>> >> const uint8_t *side_data, size_t side_data_size) >>> >> { >>> >> - AVEncryptionInitInfo *info; >>> >> + AVEncryptionInitInfo *ret = NULL, *info; >>> >> uint64_t system_id_size, num_key_ids, key_id_size, data_size, i; >>> >> + uint64_t init_info_count; >>> >> >>> >> - if (!side_data || side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA) >>> >> - return NULL; >>> >> - >>> >> - system_id_size = AV_RB32(side_data); >>> > [...] >>> >> + init_info_count = AV_RB32(side_data); >>> > >>> > i may be missing something but this looks like the meaning of the first >>> > field changes, this thus doesnt look compatible to me >>> >>> It changes the binary format of the side-data, but that was explicitly >>> not part of ABI. The fields in the structs are unchanged. This would >>> only cause a problem if the side data bytes were stored out-of-band >>> from a different version of FFmpeg. >> >> yes, right. >> its side data that is neighter a C struct nor a well defined byte stream >> its a opaque block that can only be passed to libavutil which then translates >> it into a C struct. >> its not new but it still feels clumsy to use this as means to pass data around >> > > I know it's weird, but this was the design that was decided on when > this was added. Are there any changes you want for this patch? > >> >> [...] >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> >> If you think the mosad wants you dead since a long time then you are either >> wrong or dead since a long time. >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> Ping
On Mon, May 21, 2018 at 9:25 AM, Jacob Trimble <modmaker@google.com> wrote: > On Mon, May 14, 2018 at 4:49 PM, Jacob Trimble <modmaker@google.com> wrote: >> On Tue, May 8, 2018 at 3:47 PM, Michael Niedermayer >> <michael@niedermayer.cc> wrote: >>> On Mon, May 07, 2018 at 04:59:33PM -0700, Jacob Trimble wrote: >>>> On Mon, May 7, 2018 at 3:18 PM, Michael Niedermayer >>>> <michael@niedermayer.cc> wrote: >>>> > On Mon, Apr 23, 2018 at 11:03:57AM -0700, Jacob Trimble wrote: >>>> >> While integrating my encryption info changes, I noticed a problem with >>>> >> the init info structs. I implemented them as side-data on the Stream. >>>> >> But this means there can only be one per stream. However, there can >>>> >> be multiple 'pssh' atoms in a single stream (e.g. for key rotation or >>>> >> multiple key systems). (sorry for not noticing sooner) >>>> >> >>>> >> Attached is a patch to fix this by making the init info a >>>> >> singly-linked-list. It is ABI compatible and is still easy to use and >>>> >> understand. The alternative would be to break ABI and have the >>>> >> side-data methods return an array of pointers. I could do that >>>> >> instead if that is preferable. >>>> > >>>> >> encryption_info.c | 154 +++++++++++++++++++++++++++++++++++------------------- >>>> >> encryption_info.h | 5 + >>>> >> 2 files changed, 106 insertions(+), 53 deletions(-) >>>> >> e5eecc73a6997bbd11541771372d38ea9c3972a7 0001-libavutil-encryption_info-Allow-multiple-init-info.patch >>>> >> From bb941a77e882e93629d63d63059d0063b9519e29 Mon Sep 17 00:00:00 2001 >>>> >> From: Jacob Trimble <modmaker@google.com> >>>> >> Date: Mon, 23 Apr 2018 10:33:58 -0700 >>>> >> Subject: [PATCH] libavutil/encryption_info: Allow multiple init info. >>>> >> >>>> >> It is possible for there to be multiple encryption init info structure. >>>> >> For example, to support multiple key systems or in key rotation. This >>>> >> changes the AVEncryptionInitInfo struct to be a linked list so there >>>> >> can be multiple structs without breaking ABI. >>>> >> >>>> >> Signed-off-by: Jacob Trimble <modmaker@google.com> >>>> >> --- >>>> >> libavutil/encryption_info.c | 154 +++++++++++++++++++++++------------- >>>> >> libavutil/encryption_info.h | 5 ++ >>>> >> 2 files changed, 106 insertions(+), 53 deletions(-) >>>> >> >>>> >> diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c >>>> >> index 20a752d6b4..9935c10d74 100644 >>>> >> --- a/libavutil/encryption_info.c >>>> >> +++ b/libavutil/encryption_info.c >>>> >> @@ -160,13 +160,16 @@ uint8_t *av_encryption_info_add_side_data(const AVEncryptionInfo *info, size_t * >>>> >> } >>>> >> >>>> >> // The format of the AVEncryptionInitInfo side data: >>>> >> -// u32be system_id_size >>>> >> -// u32be num_key_ids >>>> >> -// u32be key_id_size >>>> >> -// u32be data_size >>>> >> -// u8[system_id_size] system_id >>>> >> -// u8[key_id_size][num_key_id] key_ids >>>> >> -// u8[data_size] data >>>> >> +// u32be init_info_count >>>> >> +// { >>>> >> +// u32be system_id_size >>>> >> +// u32be num_key_ids >>>> >> +// u32be key_id_size >>>> >> +// u32be data_size >>>> >> +// u8[system_id_size] system_id >>>> >> +// u8[key_id_size][num_key_id] key_ids >>>> >> +// u8[data_size] data >>>> >> +// }[init_info_count] >>>> >> >>>> >> #define FF_ENCRYPTION_INIT_INFO_EXTRA 16 >>>> >> >>>> >> @@ -215,6 +218,7 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) >>>> >> for (i = 0; i < info->num_key_ids; i++) { >>>> >> av_free(info->key_ids[i]); >>>> >> } >>>> >> + av_encryption_init_info_free(info->next); >>>> >> av_free(info->system_id); >>>> >> av_free(info->key_ids); >>>> >> av_free(info->data); >>>> >> @@ -225,71 +229,115 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) >>>> >> AVEncryptionInitInfo *av_encryption_init_info_get_side_data( >>>> >> const uint8_t *side_data, size_t side_data_size) >>>> >> { >>>> >> - AVEncryptionInitInfo *info; >>>> >> + AVEncryptionInitInfo *ret = NULL, *info; >>>> >> uint64_t system_id_size, num_key_ids, key_id_size, data_size, i; >>>> >> + uint64_t init_info_count; >>>> >> >>>> >> - if (!side_data || side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA) >>>> >> - return NULL; >>>> >> - >>>> >> - system_id_size = AV_RB32(side_data); >>>> > [...] >>>> >> + init_info_count = AV_RB32(side_data); >>>> > >>>> > i may be missing something but this looks like the meaning of the first >>>> > field changes, this thus doesnt look compatible to me >>>> >>>> It changes the binary format of the side-data, but that was explicitly >>>> not part of ABI. The fields in the structs are unchanged. This would >>>> only cause a problem if the side data bytes were stored out-of-band >>>> from a different version of FFmpeg. >>> >>> yes, right. >>> its side data that is neighter a C struct nor a well defined byte stream >>> its a opaque block that can only be passed to libavutil which then translates >>> it into a C struct. >>> its not new but it still feels clumsy to use this as means to pass data around >>> >> >> I know it's weird, but this was the design that was decided on when >> this was added. Are there any changes you want for this patch? >> >>> >>> [...] >>> -- >>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >>> >>> If you think the mosad wants you dead since a long time then you are either >>> wrong or dead since a long time. >>> >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> > > Ping Added fix for issue found by Chrome's ClusterFuzz (http://crbug.com/846662). PTAL.
On Fri, May 25, 2018 at 02:16:47PM -0700, Jacob Trimble wrote: > On Mon, May 21, 2018 at 9:25 AM, Jacob Trimble <modmaker@google.com> wrote: > > On Mon, May 14, 2018 at 4:49 PM, Jacob Trimble <modmaker@google.com> wrote: > >> On Tue, May 8, 2018 at 3:47 PM, Michael Niedermayer > >> <michael@niedermayer.cc> wrote: > >>> On Mon, May 07, 2018 at 04:59:33PM -0700, Jacob Trimble wrote: > >>>> On Mon, May 7, 2018 at 3:18 PM, Michael Niedermayer > >>>> <michael@niedermayer.cc> wrote: > >>>> > On Mon, Apr 23, 2018 at 11:03:57AM -0700, Jacob Trimble wrote: > >>>> >> While integrating my encryption info changes, I noticed a problem with > >>>> >> the init info structs. I implemented them as side-data on the Stream. > >>>> >> But this means there can only be one per stream. However, there can > >>>> >> be multiple 'pssh' atoms in a single stream (e.g. for key rotation or > >>>> >> multiple key systems). (sorry for not noticing sooner) > >>>> >> > >>>> >> Attached is a patch to fix this by making the init info a > >>>> >> singly-linked-list. It is ABI compatible and is still easy to use and > >>>> >> understand. The alternative would be to break ABI and have the > >>>> >> side-data methods return an array of pointers. I could do that > >>>> >> instead if that is preferable. > >>>> > > >>>> >> encryption_info.c | 154 +++++++++++++++++++++++++++++++++++------------------- > >>>> >> encryption_info.h | 5 + > >>>> >> 2 files changed, 106 insertions(+), 53 deletions(-) > >>>> >> e5eecc73a6997bbd11541771372d38ea9c3972a7 0001-libavutil-encryption_info-Allow-multiple-init-info.patch > >>>> >> From bb941a77e882e93629d63d63059d0063b9519e29 Mon Sep 17 00:00:00 2001 > >>>> >> From: Jacob Trimble <modmaker@google.com> > >>>> >> Date: Mon, 23 Apr 2018 10:33:58 -0700 > >>>> >> Subject: [PATCH] libavutil/encryption_info: Allow multiple init info. > >>>> >> > >>>> >> It is possible for there to be multiple encryption init info structure. > >>>> >> For example, to support multiple key systems or in key rotation. This > >>>> >> changes the AVEncryptionInitInfo struct to be a linked list so there > >>>> >> can be multiple structs without breaking ABI. > >>>> >> > >>>> >> Signed-off-by: Jacob Trimble <modmaker@google.com> > >>>> >> --- > >>>> >> libavutil/encryption_info.c | 154 +++++++++++++++++++++++------------- > >>>> >> libavutil/encryption_info.h | 5 ++ > >>>> >> 2 files changed, 106 insertions(+), 53 deletions(-) > >>>> >> > >>>> >> diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c > >>>> >> index 20a752d6b4..9935c10d74 100644 > >>>> >> --- a/libavutil/encryption_info.c > >>>> >> +++ b/libavutil/encryption_info.c > >>>> >> @@ -160,13 +160,16 @@ uint8_t *av_encryption_info_add_side_data(const AVEncryptionInfo *info, size_t * > >>>> >> } > >>>> >> > >>>> >> // The format of the AVEncryptionInitInfo side data: > >>>> >> -// u32be system_id_size > >>>> >> -// u32be num_key_ids > >>>> >> -// u32be key_id_size > >>>> >> -// u32be data_size > >>>> >> -// u8[system_id_size] system_id > >>>> >> -// u8[key_id_size][num_key_id] key_ids > >>>> >> -// u8[data_size] data > >>>> >> +// u32be init_info_count > >>>> >> +// { > >>>> >> +// u32be system_id_size > >>>> >> +// u32be num_key_ids > >>>> >> +// u32be key_id_size > >>>> >> +// u32be data_size > >>>> >> +// u8[system_id_size] system_id > >>>> >> +// u8[key_id_size][num_key_id] key_ids > >>>> >> +// u8[data_size] data > >>>> >> +// }[init_info_count] > >>>> >> > >>>> >> #define FF_ENCRYPTION_INIT_INFO_EXTRA 16 > >>>> >> > >>>> >> @@ -215,6 +218,7 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) > >>>> >> for (i = 0; i < info->num_key_ids; i++) { > >>>> >> av_free(info->key_ids[i]); > >>>> >> } > >>>> >> + av_encryption_init_info_free(info->next); > >>>> >> av_free(info->system_id); > >>>> >> av_free(info->key_ids); > >>>> >> av_free(info->data); > >>>> >> @@ -225,71 +229,115 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) > >>>> >> AVEncryptionInitInfo *av_encryption_init_info_get_side_data( > >>>> >> const uint8_t *side_data, size_t side_data_size) > >>>> >> { > >>>> >> - AVEncryptionInitInfo *info; > >>>> >> + AVEncryptionInitInfo *ret = NULL, *info; > >>>> >> uint64_t system_id_size, num_key_ids, key_id_size, data_size, i; > >>>> >> + uint64_t init_info_count; > >>>> >> > >>>> >> - if (!side_data || side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA) > >>>> >> - return NULL; > >>>> >> - > >>>> >> - system_id_size = AV_RB32(side_data); > >>>> > [...] > >>>> >> + init_info_count = AV_RB32(side_data); > >>>> > > >>>> > i may be missing something but this looks like the meaning of the first > >>>> > field changes, this thus doesnt look compatible to me > >>>> > >>>> It changes the binary format of the side-data, but that was explicitly > >>>> not part of ABI. The fields in the structs are unchanged. This would > >>>> only cause a problem if the side data bytes were stored out-of-band > >>>> from a different version of FFmpeg. > >>> > >>> yes, right. > >>> its side data that is neighter a C struct nor a well defined byte stream > >>> its a opaque block that can only be passed to libavutil which then translates > >>> it into a C struct. > >>> its not new but it still feels clumsy to use this as means to pass data around > >>> > >> > >> I know it's weird, but this was the design that was decided on when > >> this was added. Are there any changes you want for this patch? > >> > >>> > >>> [...] > >>> -- > >>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > >>> > >>> If you think the mosad wants you dead since a long time then you are either > >>> wrong or dead since a long time. > >>> > >>> _______________________________________________ > >>> ffmpeg-devel mailing list > >>> ffmpeg-devel@ffmpeg.org > >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >>> > > > > Ping > > Added fix for issue found by Chrome's ClusterFuzz (http://crbug.com/846662). this belongs in a seperate patch unless its a bug specific to the code added with this patch [...]
From 039632f7b75da569f3e43780a9fa36454031f37e Mon Sep 17 00:00:00 2001 From: Jacob Trimble <modmaker@google.com> Date: Mon, 23 Apr 2018 10:33:58 -0700 Subject: [PATCH] libavutil/encryption_info: Allow multiple init info. It is possible for there to be multiple encryption init info structure. For example, to support multiple key systems or in key rotation. This changes the AVEncryptionInitInfo struct to be a linked list so there can be multiple structs without breaking ABI. Signed-off-by: Jacob Trimble <modmaker@google.com> --- libavutil/encryption_info.c | 148 ++++++++++++++++++++++++------------ libavutil/encryption_info.h | 5 ++ 2 files changed, 103 insertions(+), 50 deletions(-) diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c index 20a752d6b4..68a146953b 100644 --- a/libavutil/encryption_info.c +++ b/libavutil/encryption_info.c @@ -160,13 +160,16 @@ uint8_t *av_encryption_info_add_side_data(const AVEncryptionInfo *info, size_t * } // The format of the AVEncryptionInitInfo side data: -// u32be system_id_size -// u32be num_key_ids -// u32be key_id_size -// u32be data_size -// u8[system_id_size] system_id -// u8[key_id_size][num_key_id] key_ids -// u8[data_size] data +// u32be init_info_count +// { +// u32be system_id_size +// u32be num_key_ids +// u32be key_id_size +// u32be data_size +// u8[system_id_size] system_id +// u8[key_id_size][num_key_id] key_ids +// u8[data_size] data +// }[init_info_count] #define FF_ENCRYPTION_INIT_INFO_EXTRA 16 @@ -215,6 +218,7 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) for (i = 0; i < info->num_key_ids; i++) { av_free(info->key_ids[i]); } + av_encryption_init_info_free(info->next); av_free(info->system_id); av_free(info->key_ids); av_free(info->data); @@ -225,71 +229,115 @@ void av_encryption_init_info_free(AVEncryptionInitInfo *info) AVEncryptionInitInfo *av_encryption_init_info_get_side_data( const uint8_t *side_data, size_t side_data_size) { - AVEncryptionInitInfo *info; - uint64_t system_id_size, num_key_ids, key_id_size, data_size, i; + AVEncryptionInitInfo *ret = NULL, *info; + uint64_t system_id_size, num_key_ids, key_id_size, data_size, i, j; + uint64_t init_info_count; - if (!side_data || side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA) + if (!side_data || side_data_size < 4) return NULL; - system_id_size = AV_RB32(side_data); - num_key_ids = AV_RB32(side_data + 4); - key_id_size = AV_RB32(side_data + 8); - data_size = AV_RB32(side_data + 12); + init_info_count = AV_RB32(side_data); + side_data += 4; + side_data_size -= 4; + for (i = 0; i < init_info_count; i++) { + if (side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA) { + av_encryption_init_info_free(ret); + return NULL; + } - // UINT32_MAX + UINT32_MAX + UINT32_MAX * UINT32_MAX == UINT64_MAX - if (side_data_size - FF_ENCRYPTION_INIT_INFO_EXTRA < system_id_size + data_size + num_key_ids * key_id_size) - return NULL; + system_id_size = AV_RB32(side_data); + num_key_ids = AV_RB32(side_data + 4); + key_id_size = AV_RB32(side_data + 8); + data_size = AV_RB32(side_data + 12); - info = av_encryption_init_info_alloc(system_id_size, num_key_ids, key_id_size, data_size); - if (!info) - return NULL; + // UINT32_MAX + UINT32_MAX + UINT32_MAX * UINT32_MAX == UINT64_MAX + if (side_data_size - FF_ENCRYPTION_INIT_INFO_EXTRA < system_id_size + data_size + num_key_ids * key_id_size) { + av_encryption_init_info_free(ret); + return NULL; + } + side_data += FF_ENCRYPTION_INIT_INFO_EXTRA; + side_data_size -= FF_ENCRYPTION_INIT_INFO_EXTRA; - memcpy(info->system_id, side_data + 16, system_id_size); - side_data += system_id_size + 16; - for (i = 0; i < num_key_ids; i++) { - memcpy(info->key_ids[i], side_data, key_id_size); - side_data += key_id_size; + info = av_encryption_init_info_alloc(system_id_size, num_key_ids, key_id_size, data_size); + if (!info) { + av_encryption_init_info_free(ret); + return NULL; + } + info->next = ret; + ret = info; + + memcpy(info->system_id, side_data, system_id_size); + side_data += system_id_size; + side_data_size -= system_id_size; + for (j = 0; j < num_key_ids; j++) { + memcpy(info->key_ids[j], side_data, key_id_size); + side_data += key_id_size; + side_data_size -= key_id_size; + } + memcpy(info->data, side_data, data_size); + side_data += data_size; + side_data_size -= data_size; } - memcpy(info->data, side_data, data_size); - return info; + return ret; } uint8_t *av_encryption_init_info_add_side_data(const AVEncryptionInitInfo *info, size_t *side_data_size) { + const AVEncryptionInitInfo *cur_info, *cur_info_2; uint8_t *buffer, *cur_buffer; - uint32_t i, max_size; - - if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size || - UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA - info->system_id_size < info->data_size) { - return NULL; - } + uint32_t i, init_info_count; + + *side_data_size = 4; + init_info_count = 0; + for (cur_info = info; cur_info; cur_info = cur_info->next) { + // Detect a circular list. + for (cur_info_2 = info; cur_info_2 != cur_info; cur_info_2 = cur_info_2->next) { + if (cur_info->next == cur_info_2) + return NULL; + } + if (cur_info->next == cur_info) + return NULL; - if (info->num_key_ids) { - max_size = UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA - info->system_id_size - info->data_size; - if (max_size / info->num_key_ids < info->key_id_size) + if (UINT32_MAX == init_info_count || + UINT32_MAX - *side_data_size < FF_ENCRYPTION_INIT_INFO_EXTRA || + UINT32_MAX - *side_data_size - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size || + UINT32_MAX - *side_data_size - FF_ENCRYPTION_INIT_INFO_EXTRA - info->system_id_size < info->data_size) { return NULL; + } + *side_data_size += FF_ENCRYPTION_INIT_INFO_EXTRA + info->system_id_size + info->data_size; + init_info_count++; + + if (info->num_key_ids) { + if ((UINT32_MAX - *side_data_size) / info->num_key_ids < info->key_id_size) { + return NULL; + } + *side_data_size += info->num_key_ids * info->key_id_size; + } } - *side_data_size = FF_ENCRYPTION_INIT_INFO_EXTRA + info->system_id_size + - info->data_size + (info->num_key_ids * info->key_id_size); cur_buffer = buffer = av_malloc(*side_data_size); if (!buffer) return NULL; - AV_WB32(cur_buffer, info->system_id_size); - AV_WB32(cur_buffer + 4, info->num_key_ids); - AV_WB32(cur_buffer + 8, info->key_id_size); - AV_WB32(cur_buffer + 12, info->data_size); - cur_buffer += 16; - - memcpy(cur_buffer, info->system_id, info->system_id_size); - cur_buffer += info->system_id_size; - for (i = 0; i < info->num_key_ids; i++) { - memcpy(cur_buffer, info->key_ids[i], info->key_id_size); - cur_buffer += info->key_id_size; + AV_WB32(cur_buffer, init_info_count); + cur_buffer += 4; + for (cur_info = info; cur_info; cur_info = cur_info->next) { + AV_WB32(cur_buffer, info->system_id_size); + AV_WB32(cur_buffer + 4, info->num_key_ids); + AV_WB32(cur_buffer + 8, info->key_id_size); + AV_WB32(cur_buffer + 12, info->data_size); + cur_buffer += 16; + + memcpy(cur_buffer, info->system_id, info->system_id_size); + cur_buffer += info->system_id_size; + for (i = 0; i < info->num_key_ids; i++) { + memcpy(cur_buffer, info->key_ids[i], info->key_id_size); + cur_buffer += info->key_id_size; + } + memcpy(cur_buffer, info->data, info->data_size); + cur_buffer += info->data_size; } - memcpy(cur_buffer, info->data, info->data_size); return buffer; } diff --git a/libavutil/encryption_info.h b/libavutil/encryption_info.h index ec5501aac7..9140968fde 100644 --- a/libavutil/encryption_info.h +++ b/libavutil/encryption_info.h @@ -115,6 +115,11 @@ typedef struct AVEncryptionInitInfo { */ uint8_t* data; uint32_t data_size; + + /** + * An optional pointer to the next initialization info in the list. + */ + struct AVEncryptionInitInfo *next; } AVEncryptionInitInfo; /** -- 2.17.0.441.gb46fe60e1d-goog