matroskaenc: fix writing ProjectionPrivate element on Cubemap projections

Message ID 20170406204547.7584-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer April 6, 2017, 8:45 p.m.
Calling put_ebml_binary() with sizeof(private) as argument is currently
only valid for Equirectangular projections.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Please, be more careful when making cosmetic changes to make sure they
don't also change how the code works.
Not only this was writing invalid Cubemap files, but also dumping
uninitialized data from stack in them.

 libavformat/matroskaenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Luca Barbato April 7, 2017, 9:07 a.m. | #1
On 06/04/2017 22:45, James Almer wrote:
> Calling put_ebml_binary() with sizeof(private) as argument is currently
> only valid for Equirectangular projections.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Please, be more careful when making cosmetic changes to make sure they
> don't also change how the code works.
> Not only this was writing invalid Cubemap files, but also dumping
> uninitialized data from stack in them.
> 
>  libavformat/matroskaenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 34d983324..f6dade751 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -696,7 +696,7 @@ static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
>          avio_wb32(&b, 0); // layout
>          avio_wb32(&b, spherical->padding);
>          put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
> -                        private, sizeof(private));
> +                        private, 12);
>          break;
>      default:
>          av_log(s, AV_LOG_WARNING, "Unknown projection type\n");
> 

Probably would be safer to use avio_tell() in both cases?

lu
Vittorio Giovara April 7, 2017, 9:32 a.m. | #2
On Thu, Apr 6, 2017 at 10:45 PM, James Almer <jamrial@gmail.com> wrote:
> Calling put_ebml_binary() with sizeof(private) as argument is currently
> only valid for Equirectangular projections.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Please, be more careful when making cosmetic changes to make sure they
> don't also change how the code works.
> Not only this was writing invalid Cubemap files, but also dumping
> uninitialized data from stack in them.

Hello James,
unfortunately this mistake was present in the original patch
(https://patches.libav.org/patch/63023/) and the cosmetic change you
mention was only to break the line past the 80th column.
Thanks for the fix anyway, probably Luca's approach is more future proof though.
Hendrik Leppkes April 7, 2017, 9:54 a.m. | #3
On Fri, Apr 7, 2017 at 11:32 AM, Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
> On Thu, Apr 6, 2017 at 10:45 PM, James Almer <jamrial@gmail.com> wrote:
>> Calling put_ebml_binary() with sizeof(private) as argument is currently
>> only valid for Equirectangular projections.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Please, be more careful when making cosmetic changes to make sure they
>> don't also change how the code works.
>> Not only this was writing invalid Cubemap files, but also dumping
>> uninitialized data from stack in them.
>
> Hello James,
> unfortunately this mistake was present in the original patch
> (https://patches.libav.org/patch/63023/) and the cosmetic change you
> mention was only to break the line past the 80th column.
> Thanks for the fix anyway, probably Luca's approach is more future proof though.

The issue wasn't present in the original patch because it used a
separate (properly sized) private buffer, instead of using a shared
one for all cases.

- Hendrik
Vittorio Giovara April 7, 2017, 11:03 a.m. | #4
On Fri, Apr 7, 2017 at 11:54 AM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> On Fri, Apr 7, 2017 at 11:32 AM, Vittorio Giovara
> <vittorio.giovara@gmail.com> wrote:
>> On Thu, Apr 6, 2017 at 10:45 PM, James Almer <jamrial@gmail.com> wrote:
>>> Calling put_ebml_binary() with sizeof(private) as argument is currently
>>> only valid for Equirectangular projections.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> Please, be more careful when making cosmetic changes to make sure they
>>> don't also change how the code works.
>>> Not only this was writing invalid Cubemap files, but also dumping
>>> uninitialized data from stack in them.
>>
>> Hello James,
>> unfortunately this mistake was present in the original patch
>> (https://patches.libav.org/patch/63023/) and the cosmetic change you
>> mention was only to break the line past the 80th column.
>> Thanks for the fix anyway, probably Luca's approach is more future proof though.
>
> The issue wasn't present in the original patch because it used a
> separate (properly sized) private buffer, instead of using a shared
> one for all cases.

ok my bad then

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 34d983324..f6dade751 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -696,7 +696,7 @@  static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
         avio_wb32(&b, 0); // layout
         avio_wb32(&b, spherical->padding);
         put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
-                        private, sizeof(private));
+                        private, 12);
         break;
     default:
         av_log(s, AV_LOG_WARNING, "Unknown projection type\n");