| From: Guy Harris
|
| On Oct 28, 2003, at 5:59 AM, Biot Olivier wrote:
|
| > I figured out that there was no function yet that only
| frees the value
| > part
| > alone (indeed fvalue_free() frees the entire structure, not
| only the
| > value
| > contained within it), so I added this function to
| > epan/ftypes/ftypes.[ch] as
| > fvalue_free_value().
|
| Is that the right answer, or is the right answer to have "fvalue_set"
| functions for values that are allocated free up the old value
| if there
| is one? You can do an "fvalue_set" on values that aren't allocated,
| such as integral values, even if an "fvalue_set" has already on that
| fvalue_t; why shouldn't that be allowed for values that are allocated?
I'm not sure, but there still exists a possibility to leak memory or to dump
core when freeing memory we *think* is allocated, unless we are 100% certain
that all pointer values in the fvalue structure are always initialized to
NULL (something available via fvalue_new but I'm not certain that we always
call fvalue_new...), and that in all other cases the value may be freed
safely (and in fact must be if we don't want to leak memory).
As not all ftypes implement a memory disposal function (not all data types
of the union actually are pointers), and the code in ftypes.c checks for the
availability of the free_value function, I assume that this code is safe
(see for example fvalue_free() in epan/fvalues/fvalues.c).
My patch currently only provides functionality for FT_STRING and FT_STRINGZ
so I *am* sure that the value is referenced by a pointer. And my private
copy of the new WSP dissector (which relies on the progressive construction
of the fvalue for some more complex header fields) shows the code works, but
does not give an answer to the 1st question you rose.
Both paths can be walked anyway, and I'd be glad to provide a patch based on
the preference of the majority. But I'm just afraid that assuming that a
NULL fvalue pointer means the same as "the value has never been initialized"
may break something at unexpected parts in the code. Although it's worth
giving it a try :)
Once this functionality gets into Ethereal, I can post the new WSP
dissector.
Regards,
Olivier