Using uninitialized value ctx->pcap.binding.dumper

[okashany]

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

[amboar]

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

libmctp/utils/mctp-demux-daemon.c

Lines 668 to 676 in 8f53d63

	rc = capture_prepare(&ctx->pcap.binding);
	if (rc == -1) {
	fprintf(stderr, "Failed to initialise capture: %d\n", rc);
	rc = EXIT_FAILURE;
	goto cleanup_mctp;
	}
	mctp_set_capture_handler(ctx->mctp, capture_binding,
	ctx->pcap.binding.dumper);

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:

libmctp/utils/mctp-capture.c

Lines 19 to 42 in 8f53d63

	int capture_prepare(struct capture *cap)
	{
	int rc;
	if (cap->linktype < CAPTURE_LINKTYPE_FIRST ||
	cap->linktype > CAPTURE_LINKTYPE_LAST) {
	fprintf(stderr,
	"Invalid private linktype value %d: see https://www.tcpdump.org/linktypes.html\n",
	cap->linktype);
	return -1;
	}
	if (!(cap->pcap = pcap_open_dead(cap->linktype, UINT16_MAX))) {
	fprintf(stderr, "pcap_open_dead: failed\n");
	return -1;
	}
	if (!(cap->dumper = pcap_dump_open(cap->pcap, cap->path))) {
	fprintf(stderr, "pcap_dump_open: failed\n");
	return -1;
	}
	return 0;
	}

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.