Fix dtrace output of IP addresses as strings

[tuexen]

Summary

The IP, TCP and UDP provider provide IP addresses as strings. However, if the required data is not available, the IP and TCP provider return 0. This results in the following error when using the dtrace script

tcptest.d1 KBDownload

and a client connecting to port 1234, for which no listener exists:

   0        127.0.0.1:30003 ->        127.0.0.1:1234      40  (SYN)
dtrace: error on enabled probe ID 3 (ID 61124: tcp:kernel:none:receive): invalid address (0x0) in action #2
SYN)
dtrace: error on enabled probe ID 2 (ID 61125: tcp:kernel:none:send): invalid address (0x0) in action #2
RST|ACK)
   4        127.0.0.1:30003 <-        127.0.0.1:1234      20  (RST|ACK)

This patch suggest to use "<unknown>" instead of 0. This unbreaks dtrace:

1        127.0.0.1:42137 ->        127.0.0.1:1234      40  (SYN)
4        <unknown>:0     <-        <unknown>:0         40  (SYN)
4        <unknown>:0     ->        <unknown>:0         20  (RST|ACK)
4        127.0.0.1:42137 <-        127.0.0.1:1234      20  (RST|ACK)

For consistency, the UDP provider, which returned "" and was correct, was also changed for consistency. Since the UDP_PROBE was never triggered without providing an inp, this should not be user visible right now.

Test Plan

Use

tcptest.d1 KBDownload

in combination with a TCP client trying to connect to a non-listening port.

[dteske]

I don't agree with this change in its current iteration as it will break scripts which expect *addr to be translated to a NULL pointer should the pointer indeed be NULL. Changing the default value from a NULL pointer to an adhoc string (specifically in the context of changing it in the translator) puts the onus on script writers to test for the explicit string constant instead of NULL. This may be fine if the translator were not already established, but these are already well into their years.

The issue here is that the tcptest.d code is not using DTrace predicates nor ternary logic to test for when the information is indeed available. Like the C language, it should be considered a coding error to try and dereference a NULL pointer and like C, D provides ways to test the pointer before dereference to prevent such situations.

The errors cited in the review text give much information with respect to the invalid dereferencing:

dtrace: error on enabled probe ID 3 (ID 61124: tcp:kernel:none:receive): invalid address (0x0) in action #2
dtrace: error on enabled probe ID 2 (ID 61125: tcp:kernel:none:send): invalid address (0x0) in action #2

The first dereference of a NULL pointer comes in the form of probe ID 3 and the second one in probe ID 2. These probe identifiers are numerically assigned based on the probes in tcptest.d (given below):

probe ID 1: dtrace:::BEGIN { ... }
probe ID 2: tcp:::send { ... }
probe ID 3: tcp:::receive { ... }
probe ID 4: tcp:::send, tcp:::receive { ... }

If you're seeing the above invalid address (0x0) for probe ID 2 and 3, it's because you're trying to dereference a NULL pointer. Further, if changing the translators in /usr/lib/dtrace/*.d -- specifically the *addr members -- assuages those errors, it's likely the act of printf trying to reach into NULL to get a string. This may be obvious, but it's important to enumerate it for the following discussion.

The appropriate fix for tcptest.d would not be altering the translators (which are relied upon by scripts both in the wild and in base starting with FreeBSD 11.2 -- see /usr/libexec/dwatch/tcp for example), but you should add ternary conditions to your probes that prevent dereferencing a NULL pointer. For example, change the original code (below) ...

tcp:::send
{
	printf(" %3d %16s:%-5d -> %16s:%-5d %6d  (", cpu,
	    args[3]->tcps_laddr, args[3]->tcps_lport,
	    args[3]->tcps_laddr, args[3]->tcps_rport, /* WARNING: You have a typo on this line, did you mean tcps_raddr? */
	    args[2]->ip_plength);
}

tcp:::receive
{
	printf(" %3d %16s:%-5d <- %16s:%-5d %6d  (", cpu,
	    args[3]->tcps_laddr, args[3]->tcps_lport,
	    args[3]->tcps_raddr, args[3]->tcps_rport,
	    args[2]->ip_plength);
}

To the following which allows you to define your own fallback:

tcp:::send
{
	printf(" %3d %16s:%-5d -> %16s:%-5d %6d  (", cpu,
	    (addr = args[3]->tcps_laddr) != NULL ? addr : "<unknown>",
	    args[3]->tcps_lport,
	    (addr = args[3]->tcps_raddr) != NULL ? addr : "<unknown>",
	    args[3]->tcps_rport,
	    args[2]->ip_plength);
}

tcp:::receive
{
	printf(" %3d %16s:%-5d <- %16s:%-5d %6d  (", cpu,
	    (addr = args[3]->tcps_laddr) != NULL ? addr : "<unknown>",
	    args[3]->tcps_lport,
	    (addr = args[3]->tcps_raddr) != NULL ? addr : "<unknown>",
	    args[3]->tcps_rport,
	    args[2]->ip_plength);
}

This was just a simple example, you can actually centralize the fallback to either a global, thread-local, or clause-local variable. You can also create your own translators or map functions. Either way, changing the core translators in /usr/lib/dtrace to change what they return is assuredly the wrong approach.

[tuexen]

I guess I was not clear enough what the issue is that I'm trying to address. I'm not trying to get the script running. It was just used to demonstrate the issue.

The IP, TCP and UDP provider provide addresses as strings. There are cases where no strings can be computed like there is no tcpcb in the case of TCP. What value should be provided in that case?
In the case of IP and TCP, a NULL pointer is provided, in the case of UDP an empty string is provided. This is not consistent! I think this should be fixed.

So I tried to figure out what should be returned in this case. Solaris always returns a string which is "<unknown>" in this case. The script provided works on Solaris. This string is also used in other places in FreeBSD.

Newer versions of MacOS do provide network providers. There the string "null" is returned in this case (for IP only, since they don't have addresses in tcpsinfo_t and don't have a UDP provider).

So the question is: What should be returned when the address as a string can't be computed by a network provider?
Should it either be a NULL pointer (in which case we need to fix udp.d) or a string? If it should be a string, which one? In case it should be the empty string, we need to change ip.d and tcp.d, if it should be some non-empty string (like "<unknown>") we need to change ip.d, tcp.d. and udp.d. I don't care that much which value is used in this case, as long as it is consistent. However, I would prefer a string to the NULL pointer, since other implementation always return a string.

Just to provide the context why I'm looking into this: I'm implementing the SCTP network provider based on Solaris 11.4 DTrace Guide and try to understand how this should work. Since there are some issue with the specification I'm testing some edge cases on Solaris. That is why I'm also looking at the UDP and TCP provider to get a consistent behaviour (dtrace on Solaris is consistent and therefore it seems reasonable for dtrace on FreeBSD to also be consistent).

[dteske]

Thanks for the explanation, cheers!

[gnn]

LGTM. Thanks.