On Wed, 2016-08-10 at 18:33 +0200, Peter Meerwald-Stadler wrote:
> Hi,
>
> >
> > Using pa_xfree() irrespective of whether old_value is NULL or not
> > to
> > avoid potentially confusing nest of if statements.
>
> starring at the same issue ATM :)
> the proposed fix below gets rid of the potential leak, butÂ
>
> (1) why do we set old_value to "(data)" in the first place?
> wouldn't "(null)" make more sense?
pa_proplist_contains() returned true, so the hashmap contains something
for the key. old_data can only be NULL, if the old value was not an
utf8 string, hence "(data)" instead of the actual value.
> (2) in case value == 0, we'd still overwrite old_value, makes no
> sense
Why not? value is the new value, old_value is the old value. The new
value doesn't affect what we should print in the log message for the
old value.
> (3) could use pa_safe_streq()
>Â
> my suggestion would be
>
> Â Â Â Â if (pa_proplist_contains(o->proplist, key)) {
> Â Â Â Â Â Â Â Â old_value = pa_xstrdup(pa_proplist_gets(o->proplist, key));
> Â Â Â Â Â Â Â Â if (str_safe_streq(value, old_value))
> Â Â Â Â Â Â Â Â Â Â Â Â goto finish;
This doesn't handle correctly the transition from non-utf8 data to
unset (i.e. when both variables are NULL).
--Â
Tanu
