[Coverity CID: 220541] Dereference before null check in subsys/net/lib/capture/capture.c

[zephyrbot]

Static code scan issues found in file:

https://github.com/zephyrproject-rtos/zephyr/tree/b86f7addae05add0db45d9b528854235fbb93a48/subsys/net/lib/capture/capture.c#L342

Category: Null pointer dereferences
Function: net_capture_setup
Component: Networking
CID: 220541

Details:

zephyr/subsys/net/lib/capture/capture.c

Line 342 in b86f7ad

if (remote_iface == NULL) {

336         } else {
337             NET_ERR("Invalid address family %d", remote.sa_family);
338             ret = -EINVAL;
339             goto fail;
340         }
341    
>>>     CID 220541:  Null pointer dereferences  (REVERSE_INULL)
>>>     Null-checking "remote_iface" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
342         if (remote_iface == NULL) {
343             NET_ERR("Remote address %s unreachable", remote_addr);
344             ret = -ENETUNREACH;
345             goto fail;
346         }
347    

Please fix or provide comments in coverity using the link:

https://scan9.coverity.com/reports.htm#v29271/p12996

Note: This issue was created automatically. Priority was set based on classification
of the file affected and the impact field in coverity. Assignees were set using the CODEOWNERS file.

[mniestroj]

I think that there should be if (!remote_iface) check added just after remote_iface assignment:

		iremote_iface = net_if_ipv6_select_src_iface(
						&net_sin6(&remote)->sin6_addr);
		if (!remote_iface) {
			NET_ERR("Remote address %s unreachable", remote_addr);
			return -ENETUNREACH;
		}

and

		remote_iface = net_if_ipv4_select_src_iface(
						&net_sin(&remote)->sin_addr);
		if (!remote_iface) {
			NET_ERR("Remote address %s unreachable", remote_addr);
			return -ENETUNREACH;
		}
`
``

[jukkar]

I considered that solution but decided to do it differently. If both IPv4 and IPv6 are enabled, there would be the same check twice which is unnecessary. So I placed the remote_iface NULL check after the IPv4/IPv6 checks.

[mniestroj]

There would be two checks in compiled binary, yes. But it is hard to do without it before using net_if_get_mtu(). And I don't think returning 0 from net_if_get_mtu(NULL) (as proposed in jukkar@83db5ca) is a good idea. With this code:

		orig_mtu = net_if_get_mtu(remote_iface);
		mtu = orig_mtu - sizeof(struct net_ipv6_hdr) -
			sizeof(struct net_udp_hdr);

mtu will end up being negative value. In most cases (including that one) returning 0 from NULL interface won't be much of an improvement, so I would rather keep net_if_get_mtu(NULL) behavior undefined, as NULL should not be used in the first place.

[mniestroj]

If both IPv4 and IPv6 are enabled, there would be the same check twice which is unnecessary

And as noted above, this check would be twice in compiled binary. However for a single function invocation, there would be just a single check.

[mniestroj]

What about calculating ip headers first (based on IPv4 vs IPv6):

	ip_headers_len = sizeof(struct net_ipv6_hdr);

or

	ip_headers_len = sizeof(struct net_ipv4_hdr);

and then mtu after remote_iface check:

	if (remote_iface == NULL) {
		NET_ERR("Remote address %s unreachable", remote_addr);
		ret = -ENETUNREACH;
		goto fail;
	}

	mtu = net_if_get_mtu(remote_iface) - ip_headers_len -
		sizeof(struct net_udp_hdr);

[jukkar]

mtu will end up being negative value.

The mtu value (if <0) is not used as we bail out if remote_iface is NULL.