Use-after-free under icaltimezone_convert_time()

[Milan Crha]

On Tue, 2017-10-17 at 15:34 +0200, Robert Stepanek wrote:
> Do I understand correctly that *all* timezone structs returned
> currently by libical (with get_builtin_timezone) are not thread-safe?

	Hi,
at least with respect of icaltimezone::changes, yes. To be fair, I
never thought of libical as being 100% thread safe, also because many
structures are not allocated on heap, but on stack instead.

> That is, it's not just an effect of the special way how Evolution
> interacts with libical?

Correct. I can write a little test program which will call libical
functions only, not those icaltimezone directly, but to convert times
instead, in multiple threads with the same zone instance as a
reproducer, if it would help. That's basically the only "specialty" on
the evolution(-data-server) side, that it uses one instance of the
structure in multiple threads at the same time (without even noticing).

> That would be important to know for developers, shouldn't we
> put that in the documentation?

Not a question for me.

> Obviously, callers would need to take care of freeing the timezone,
> so we might define this as
> icaltimezone_get_builtin_r or the like.

I wouldn't do that. Memory management of libical is somewhat special,
many structures are passed as arguments, instead of pointers, not
talking about thread-related memory pools and similar things.
Evolution(-data-server) takes advantage of this memory model, thus any
such change would mean significant changes on the evolution(-data-
server) side to properly address this thread-safety issue.

The locking feels much better and simpler for the libical users, from
my point of view. The only problem (without locking) is that this
breaks when there's required to extend the 'changes' array interval,
otherwise the array is left unchanged and multi-threaded applications
work without any issue. I did think of read-write lock, instead of a
mutex, which might theoretically speed things up, but then I decided to
use a simpler thread synchronization primitive. I can change it, if
needed.
	Thanks and bye,
	Milan