Use-after-free in 2.0.0 under icalrecur_iterator_new()

[Milan Crha]

On Thu, 2016-02-18 at 17:01 -0500, Ken Murchison wrote:
> Rather than creating a shared pool, wouldn't it be easier to just 
> dynamically allocate rscale for each icalrecurrencetype (not out of the 
> shared icalmemory pool)?

	Hi,
the "not out of the shared icalmemory pool" confuses me. Having in the
structure a newly strdup()-ed memory would mean to provide
an icalrecurrencetype_free(), beside
existing icalrecurrencetype_clear(), which you probably do not want to.
That probably means that the (for me) confusing sentence means: make it
a dynamically allocated memory inside the shared icalmemory pool?

I read the libical code only briefly, but if I understand it correctly,
then the tmp buffers are temporary in a meaning that they can be freed
any time, and if libical is built with pthread, then these buffers do
not survive thread end (the tmp buffers are allocated for each thread
separately). That is not the case for the icalmemory buffer, which is
shared between threads and its members are freed explicitly. Right?
Overall libical memory management is sometimes confusing on its own,
definitely regarding the tmp buffers.

> Or rscale could just be a fixed length buffer, 
> assuming that the name from ICU have a documented maximum length.

That would make it too, but you know users, they can write anything
into that parameter and libical should preserve it, even it's a
nonsense. There could be a problem with the buffer, if the maximum
length is given from the ICU headers and it would eventually change in
the future, which would be an ABI change for the libical (if it would
be built against two different ICU libraries, one with smaller and one
with bigger value).

I do not know what is better here. The libical API says that
the icalrecurrencetype structure can be used without allocating it by
any explicit icalrecurrencetype_new() and without explicitly freeing
it. This new char *rscale member complicates things a bit. You need to
make sure that threading is not a problem, the same as the rscale will
not be lost on its own.

My patch chose a safe solution, according to my limited knowledge of
the libical.

	Bye,
	Milan