libmctp icon indicating copy to clipboard operation
libmctp copied to clipboard

Using uninitialized value ctx->pcap.binding.dumper

Open okashany opened this issue 2 years ago • 1 comments

In mctp-demux-daemon.c line #885 ctx->pcap.binding.dumper is being used although it is uninitialized.

okashany avatar Nov 14 '22 06:11 okashany

@okashany FWIW you can actually link to the code directly and github renders it nicely:

https://github.com/openbmc/libmctp/blob/8f53d6310bd166670b123b982c38b32e8a3a877b/utils/mctp-demux-daemon.c#L668-L676

That said there's no line 885 at the tip of master. I think I've highlighted the correct spot. Can you please verify?

If it is the spot you're concerned about, then by my reasoning it appears to be a false positive. We can only call mctp_set_capture_handler() if capture_prepare() has returned a value that's not -1. capture_prepare() is as follows:

https://github.com/openbmc/libmctp/blob/8f53d6310bd166670b123b982c38b32e8a3a877b/utils/mctp-capture.c#L19-L42

We see that all early-exit error paths return -1 despite dumper being initialised last. Assuming the code otherwise has well-defined behaviour, any error in capture_prepare() will prevent mctp_set_capture_handler() from being invoked due to the error handling at the call-site, and dumper is always properly assigned if there is no error.

I'm in two minds about assigning dumper in order to silence a false-positive from any static analysis tool that might have detected this on the basis that the code isn't incorrect and it's not particularly unsafe due to the existing error handling. That said, I don't want to give the impression that I don't want any insights from static analysis reported. It's just unfortunate that if my reasoning is correct that such tools don't have the power to identify this as a false positive.

amboar avatar Nov 14 '22 23:11 amboar