Comment 10 for bug 1998461

Revision history for this message
Robie Basak (racb) wrote :

Thank you for your help with this!

This looked like it was ready to release in Jammy, but I noticed a couple of things that should probably be addressed first.

1) Kinetic isn't verified yet, and so if we release this in Jammy, users might experience a regression on upgrade to Kinetic. Ideally we'd release the fix to Kinetic before or at the same time.

2) I looked at the patch itself, and found it concerning:

netsnmp_parse_ep_str() is called multiple times in succession, for example from snmpIPv4BaseDomain.c once for "default_target" and again against "inpeername" in the "Invalid default target" failure case. However:

a) While the patch adds various guards in the case that ep_str.addr is NULL, the error exit path in netsnmp_parse_ep_str() frees the pointer but does not reset ep_str.addr back to NULL. So what will happen later?

b) Given that netsnmp_parse_ep_str() can be called multiple times, what happens when it allocates new memory to ep_str.addr again? Is this a memory leak?

It's not clear to me if either of the paths above are actually possible to trigger from outside the program. That would require further analysis to find how these functions are called. But it seems to be that, at a minimum, the guards aren't complete unless all free() calls on the pointer reset the field to NULL, and that nothing new should be assigned to ep_str.addr before first freeing it if it was formerly not NULL.

I think this code needs to be more mature upstream before we can accept it as a patch in an SRU in Ubuntu.

On the other hand if you think my analysis is flawed, I would be happy to be corrected.