On 09/27/2017 09:34 AM, Erik Skultety wrote:
> On Wed, Sep 27, 2017 at 08:06:42AM -0400, John Ferlan wrote:
>>
>> $subj:
>>
>> nwfilter: Fix memory leak in learnIPAddressThread
>>
>> On 09/26/2017 09:01 PM, ZhiPeng Lu wrote:
>>> In learnIPAddressThread()the @inetaddr may be leaked.
>>>
>>
>> Changing this to:
>>
>> Don't leak @inetaddr within the done: processing when attempting
>> to instantiate the filter.
>>
>>
>>> Signed-off-by: ZhiPeng Lu <lu.zhipeng@xxxxxxxxxx>
>>> ---
>>> src/nwfilter/nwfilter_learnipaddr.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
>>> index cfd92d9..0cadf73 100644
>>> --- a/src/nwfilter/nwfilter_learnipaddr.c
>>> +++ b/src/nwfilter/nwfilter_learnipaddr.c
>>> @@ -605,6 +605,7 @@ learnIPAddressThread(void *arg)
>>>
>>> if (req->status == 0) {
>>> int ret;
>>> + int mapipret = -1;
>>> virSocketAddr sa;
>>> sa.len = sizeof(sa.data.inet4);
>>> sa.data.inet4.sin_family = AF_INET;
>>> @@ -622,7 +623,7 @@ learnIPAddressThread(void *arg)
>>> virNWFilterUnlockIface(req->ifname);
>>>
>>> if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) {
>>> - if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) {
>>> + if ((mapipret = virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr)) < 0) {
>>> VIR_ERROR(_("Failed to add IP address %s to IP address "
>>> "cache for interface %s"), inetaddr, req->ifname);
>>> }
>>> @@ -637,6 +638,9 @@ learnIPAddressThread(void *arg)
>>> req->filterparams);
>>> VIR_DEBUG("Result from applying firewall rules on "
>>> "%s with IP addr %s : %d", req->ifname, inetaddr, ret);
>>> + if (mapipret < 0)
>>> + VIR_FREE(inetaddr);
>>> +
>>
>> What's the purpose of mapipret? @inetaddr isn't allocated because of
>> the return of virNWFilterIPAddrMapAddIPAddr it's allocated because of
>> virSocketAddrFormat and thus would need to be VIR_FREE'd regardless.
>>
>> I've fixed that by just removing the unnecessary @mapipret and just
>> using VIR_FREE() at the end of the if statement.
>
> That's a wrong fix, because as ZhiPeng Lu noted in previous version, you freed
> something that is assigned to the ipAddressMap pool on success, so the next
> time you access it, you'll get a SIGSEGV
>
Ugh! My brain couldn't parse what he wrote. I'm glad you could.
Still this pile of code is awful....
virNWFilterIPAddrMapAddIPAddr takes @addr and if not found in
ipAddressMap->hashTable, it creates a @val from @addr and inserts @val
into a hash table and the caller can/should free @addr on success.
If it is found, then virNWFilterVarValueAddValue is called and on
success, yes, it consumes @addr and caller shouldn't free @addr
So how is the caller to know that @addr has been consumed or not on
success. For one instance it is, but on the other it isn't. Certainly
on failure it's not consumed and free-able.
> Erik
>
> Btw I wanted to respond to the previous version in the morning, but I couldn't
> come up with anything else than either using STRDUP in
> virNWFilterIPAddrMapAddIPAddr, so then we could go with the proposal you just
> pushed, or we could refactor the whole nwfilter as it's a real mess...(that's a
> joke btw, I don't think anyone is volunteering to that any time soon) - it's a
> shame that the nwfilter code is kinda "no touchy" unless something crashes.
>
So I think the fix to this awful code would be to use VIR_STRDUP() in
virNWFilterIPAddrMapAddIPAddr for the else condition into a @tmp
variable that would be used in call to virNWFilterVarValueAddValue. I'll
send something shortly.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
