[FFmpeg-cvslog] r17011 - trunk/libavformat/mxfenc.c

[Baptiste Coudurier]

Hi Reimar,

On 2/6/2009 1:06 AM, Reimar D?ffinger wrote:
> On Thu, Feb 05, 2009 at 04:33:02PM -0800, Baptiste Coudurier wrote:
>> On 2/5/2009 3:46 PM, Reimar D?ffinger wrote:
>>> You can't know if this is security-critical without knowing what the
>>> application using ffmpeg does.
>>> Fact is that it causes a side-effect which changes global state - other
>>> functions like random() were forbidden for this reason.
>> I'm not so familiar with thread safety, so you may be right. Can you
>> explain a potential harmful scenario ?
>
> A particularly plausible one? No.
> Just some scenario? Yes.
> Some application could implement a kind of certificate check and wants
> to check if it is still valid. Due to some design idiocy it does that
> via localtime(time) (yes, this part is very contrived. It is correct
> that localtime should not be used by almost anything except maybe stuff
> displayed to the user).
> It does that in a separate thread to allow encoding at the same time.
> Right after that localtime finishes, there is a timer interrupt and now
> the libavformat thread runs, calling localtime to convert the MXF
> timestamp (which refers to a date some years back).
> Some time later the checking thread runs again and will check the
> end-of-validity date against the years-old date of the video instead of
> the current date, since the localtime call in libavformat overwrote the
> buffer.

Thanks for the example. I see a bit more how it can hurts now.

> [...]
>> I can understand the thread-local store, here people using the virtual
>> mechanism are really looking into problems by using localtime.
>
> Actually, forget about that. Thread-local store will break down with
> exactly the same problems and more when you pass the pointer returned
> by localtime to a different thread, which is not forbidden
> (although less likely).
>
>>> Btw. I am against localtime because that means when transcoding (once
>>> we support transferring this information) the result will depend on
>>> the local time zone (even when both source and destination are MXF, due
>>> to summer time, conversion localtime ->   time_t ->   localtime is not
>>> lossless - I think).
>> For now, since my application reads the time "as is", Im against using
>> GMT time, unless stated in specs.
>
> Local time has serious issues, and usually has values of 1 hour once a
> year it can not represent and once a year that is ambiguous (the later one
> meaning it is not really suitable for e.g. security cameras), and those differ
> by country.
> Using localtime is a huge hack, and IMO that should be in your
> application if you want it.
> It is easy to do this hack in your application, just do
> timestamp = timegm(localtime(time)) instead of
> timestamp = time
> You will still have all the issues (including all the fun should you
> ever use "cloud computing" where randomly a server might be located in a
> different timezone), but at least other users will be able
> to do things in a non-broken way without having to change their timezone
> to UTC.
> A somewhat nicer way might be to add a "struct tm" instead of just the
> timestamp by the user, this would also allow to encode values before and
> after the unix epoch which I think currently is not possible.

I understand your concerns, and they are mostly valid.
Now one user already complained that muxer was not setting a correct and 
understandable time. I want this fixed.
I understand the implication of using localtime, so I will use 
localtime_r for thread safety.

However how can I explain that time displayed is not the correct one ? 
While every other application creating mxf around are doing it 
"somewhat" correctly.

This is not reasonable. If people have problem with local time, as 
people using mxf are mainly industry people, they can complain to SMPTE 
to provide a mechanism to supply time zone. Standardization screwed up, 
people have to deal with it.

Furthermore, many muxers are using the value returned from parse_time 
which uses localtime already, so far _nobody_ complained.

Adding struct tm, as an API extension is an idea, however this imply 
adding some more options and parsing code, and I don't feel like it atm.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org