utils: fix leaks for map unpacking

[DavidKorczynski]

Fixes several leaks in ctraces, including:

• https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=54530
• https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=54573
• https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=57387

[DavidKorczynski]

I don't think the fix is the generally best solution but would prefer if some maintainers adjust for the right leak.

Specifically, it's not a general solution because the leaks happens other values set similarly across ctr_decode_msgpack and the key issue seems ot be ctr_mpack_unpack_map fails to clean up in situations where some decoding is successful and some is unsuccessful

[leonardo-albertovich]

Please move the definition of context_to_free to the beginning of the function and reneame it as context_to_release if possible while at it.

[leonardo-albertovich]

Actually, I'm sorry David, even though the style request was technically correct, I didn't pay attention to what the change actually was.

I think the change is wrong because the conditional branch you put that cleanup in is something that should be handled by the caller.

What happens when the map key handler returns anything but CTR_MPACK_SUCCESS is that error should be elevated and each scope should perform the necessary cleanup for things that aren't handled by an outer function.

From what I can see the issue in this case is that in line 569 we shouldn't return the result of the ctr_mpack_unpack_map call but instead save it in a local variable and if it's not CTR_MPACK_SUCCESS we should use ctr_span_destroy to dispose of context->span and set it to NULL.

Please correct me if I'm wrong or let me know if you have any questions.

[DavidKorczynski]

I tried this Leonardo, but it didn't work and in general I don't think it's possible to have the fix there.

The problem is that the leak happens in a loop where the loop itself doesn't have knowledge of the underlying element, which in this case has the consequence it cannot free all data associated with the element. This is what I meant from the initial PR text This is because the dynamically allocated memory attached to context which is typecasted in the callees but is never casted back in case of needing to clean up..

I think in essence there is missing information available when unpacking map/arrays and this makes it tricky to clean it up. The problem is if parts of the elements in the map/arrays are succesfully unpacked but the unpacking later fails, then it seems like leaks happen. I think this extends further than spans as well.

[leonardo-albertovich]

Could you please share the patch you applied so I can assist you with it? It's probably something simple because I'm fairly sure that it can be addressed in this way.

[leonardo-albertovich]

@edsiper this PR should be merged ASAP.

Should @DavidKorczynski increase the patch level in this PR or will we do it afterwards?

[edsiper]

@leonardo-albertovich pls bump it, thank you