Add some functional tests for tftpd(8)

[asomers]

Summary

Add some functional tests for tftpd(8)

tftpd(8) is difficult to test in isolation due to its relationship with
inetd. Create a test program that mimics the behavior of tftp(1) and
inetd(8) and verifies tftpd's response in several different scenarios.

[cem]

Just a few nits; I haven't made it through functional.c yet.

etc/mtree/BSD.tests.dist

384–385 ↗

(On Diff #39162)

Should this be "tftpd?"

libexec/tftpd/tests/Makefile

7 ↗

(On Diff #39162)

Isn't this the default WARNS?

libexec/tftpd/tests/functional.c

2 ↗

(On Diff #39162)

Might want to throw in an SPDX while you're here.

36 ↗

(On Diff #39162)

I'd sort the non-standard library header after the standard ones, like atf-c.h below.

49 ↗

(On Diff #39162)

Unless you intend this to be a variadic function, use cleanup(void).

[asomers]

etc/mtree/BSD.tests.dist

384–385 ↗

(On Diff #39162)

Of course.

libexec/tftpd/tests/Makefile

7 ↗

(On Diff #39162)

Not for tests. The default WARNS for tests is 0. It probably should be 6 instead, but that's a separate issue.

[asomers]

Respond to @cem's comments

[cem]

libexec/tftpd/tests/functional.c

60–61 ↗

(On Diff #39200)

This isn't safe — buffer isn't zero initialized and read() doesn't have to fill it with a nul-terminated string. Quick fix is char buffer[80] = {0};

63 ↗

(On Diff #39200)

Using procctl(2) and reapers might be better for this sort of thing in the long term. Not something you need to change for this, though.

76 ↗

(On Diff #39200)

style nit: spaces go between various punctuation and letters here

76–77 ↗

(On Diff #39200)

Didn't want to just use memcmp()?

90 ↗

(On Diff #39200)

Is tftp ipv4 only?

97 ↗

(On Diff #39200)

style nit: asterisk goes after space

111 ↗

(On Diff #39200)

What does it mean to bind() with a zero sin_addr? I guess it's just implicitly INADDR_ANY?

121 ↗

(On Diff #39200)

Doesn't this mechanism seem unnecessarily brittle, given the parent knows the child pid after fork() already? We can avoid round-tripping it via pidfile and instead just store it in a global.

126 ↗

(On Diff #39200)

This seems redundant given we zeroed addr above. Also, addr doesn't seem to be used after this point.

132 ↗

(On Diff #39200)

I don't know how tftpd works, but it seems a little odd to redirect stderr over the same socket as stdout. Maybe stderr should just be a handle to /dev/null instead?

160 ↗

(On Diff #39200)

I think you can drop the outer const void * cast and rely on implicit conversion to const void *.

Or just create a const char * alias of parameter buf to avoid all of the casting.

183 ↗

(On Diff #39200)

This would be a good use for the __COUNTER__ language extension.

200 ↗

(On Diff #39200)

I'd use O_RDONLY explicitly as well. (This happens to work because O_RDONLY has the value zero.)

206 ↗

(On Diff #39200)

Why not just type r correctly as ssize_t? The value of r (with the right type) should be checked for >= 0.

Also, s is unconnected and unbound — can't recvfrom just receive any old UDP message the system happens to get? Seems fragile.

209–210 ↗

(On Diff #39200)

What's the function of this change? Can the tftp server respond from a different port than we transmitted to?

226 ↗

(On Diff #39200)

A lot of code duplication with the prior routine here. Seems like a general helper routine could:

1. Take as parameters (via struct):
   1. Command, expected response, ack buffers, and their sizes.
   2. File name and contents
2. Write out the file per parameters;
3. Validate the command sends, the expected response is received, and the ack sends.

279 ↗

(On Diff #39200)

Ditto with the code duplication.

308 ↗

(On Diff #39200)

style nits: spacing, s/i=i+1/i++/

[asomers]

Thanks for the very detatiled review!

libexec/tftpd/tests/functional.c

63 ↗

(On Diff #39200)

Cool. I didn't know about that. I'll look into it.

76–77 ↗

(On Diff #39200)

No, because memcmp won't provide a useful error message. It's helpful to know where in the packet the miscompare happened.

90 ↗

(On Diff #39200)

No, it works over IPv6 too. I'll try to add it in a later version.

111 ↗

(On Diff #39200)

Yes. INADDR_ANY is defined to be all zeros.

121 ↗

(On Diff #39200)

Nope, because ATF cleanup functions get called from a separate process. Globals don't persist between the test's body and its cleanup.

126 ↗

(On Diff #39200)

Yeah, I think that line is leftover from a previous iteration of the code.

132 ↗

(On Diff #39200)

Indeed it is odd. But that's what inetd does. It's documented in the man page, and you can see it in inetd.c:807. I don't think tftpd uses stderr. It just logs to syslog instead.

160 ↗

(On Diff #39200)

Huh. I don't know why I didn't come up with that to begin with.

183 ↗

(On Diff #39200)

Yes! Unfortunately, I don't think it's available in GCC 4.2. Maybe I should just disable the test entirely for such builds?

206 ↗

(On Diff #39200)

Probably my Rust instincts kicking in, telling me that array indices must always be usize. I'll change it.

Regarding s, I believe that the act of calling sendto implicitly assigns a port to the socket, and it is to that port that tftpd directs its reply. So s could indeed receive any packet directed to that random port. Such is the nature of UDP. Fixing that would probably require creating an epair of interfaces and a separate FIB.

209–210 ↗

(On Diff #39200)

Exactly. That's how the protocol works. I think it's how tftp fakes connections, since it doesn't use TCP. The first packet of a command always goes to port 69, and the server's reply includes the port to which subsequent packets should be directed.

226 ↗

(On Diff #39200)

I know. It bothers me too. But I failed to come up with a good way to separate the helper that was still flexible enough for things like error injection. I'll keep trying.

[cem]

libexec/tftpd/tests/functional.c

63 ↗

(On Diff #39200)

It may be more effort than it is worth for an individual test, but in general (maybe with a wrapper abstraction) it should replace pid-based mechanisms cleanly.

63 ↗

(On Diff #39200)

And maybe ignore this -- I don't think that it will work with separate parent and cleanup processes.

76–77 ↗

(On Diff #39200)

That's fair. It might also be useful to hexdump the whole packet contents (or the first N bytes, N ~= 32 or so)?

Something like: "Miscompare at position %lu: Expected: <hexdump> Actual: <hexdump>".

Feel free to skip this suggestion, I'm mostly just thinking out loud.

90 ↗

(On Diff #39200)

It shouldn't be too hard to pass around generic sockaddr pointers instead of sockaddr_in. And actual storage will be replaced with sockaddr_storage.

121 ↗

(On Diff #39200)

Ah, ok.

132 ↗

(On Diff #39200)

Ok

183 ↗

(On Diff #39200)

You're right about GCC4.2; it shows up in later GCC4.x apparently. :-(

I would be fine with disabling the test on GCC4.2.

206 ↗

(On Diff #39200)

I don't see anything in the sendto(2) or recvfrom(2) manual pages that leads me to believe the socket is left in a state where it only receives packets on a single port, but maybe the pages are incomplete.

The only documentation I can find to that effect is actually in Windows:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms740120(v=vs.85).aspx

The local address of the socket must be known. ... Explicit binding is discouraged for client applications. For client applications using this function, the socket can become bound implicitly to a local address through sendto, ...

I don't see any documentation to that effect in POSIX:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/recvfrom.html

As far as I can tell, that socket can receive any old datagram socket to any old IP or port, since it isn't bound.

209–210 ↗

(On Diff #39200)

Got it. Can you add a comment to that effect? :-)

[asomers]

libexec/tftpd/tests/functional.c

206 ↗

(On Diff #39200)

It can't. If it could, then it could receive packets destined for ports < 1024, and it isn't allowed to do that. In fact, I don't even know of a way to make a socket receive packets destined to multiple ports. How would you know the destination port of a given packet? There's no "recvto" system call that would tell you that info. The sendto implicitly binds the socket. That's why client applications don't need to worry about port conflicts with other client programs.

I can't find any docs that explicitly state this for sendto. However, connect does the exact same thing for TCP sockets, and the opengroup docs _do_ state it explicitly.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/connect.html

226 ↗

(On Diff #39200)

I'm working on a DSL that should take care of this. It'll handle both IPv4 and IPv6 too.

[cem]

libexec/tftpd/tests/functional.c

206 ↗

(On Diff #39200)

You can connect(2) datagram sockets, but this one isn't. And UDP is definitely not a connection-oriented protocol (sometimes called out separately in the manual pages).

So anything talking about connected sockets or connection-oriented protocols isn't immediately obviously applicable here (at least, to me). I don't see any language to the effect that sendto() makes a socket connected.

I would love to be wrong. If so, it would be *very* good to clarify the documentation (I'm volunteering here).

Explicitly using connect(2) would also assuage my concerns about the code (if not the documentation).

226 ↗

(On Diff #39200)

Thanks

[asomers]

Make a DSL for tftpd test cases. It makes generous use of macros and global
variables to substantially simplify individual test cases. It also
abstracts away the differences between IPv4 and IPv6.

[cem]

I'm really not a fan of the giant macro approach. I was hoping for a data-driven (i.e., structures/tables of expected communication sequences) approach. My reading of the existing tests is that it is quite possible to do so.

[asomers]

In D14310#302269, @cem wrote:

I'm really not a fan of the giant macro approach. I was hoping for a data-driven (i.e., structures/tables of expected communication sequences) approach. My reading of the existing tests is that it is quite possible to do so.

I think there are three things that I can't currently do without macros:

1. automatically determine the size of an array argument, like RECV and SEND_STR do. If I were to turn those into functions, then individual test cases would need a lot more sizeof statements.
2. concatenate string arguments, like SEND_RRQ and SEND_WRQ do.
3. Automatically handle the ATF boilerplate, as TFTPD_TC_DEFINE does. Without macros, I would need about 31 more lines per test case.

I'm not seeing a cleaner way to do this by defining data driven test cases. The ATF boilerplate, especially, would take a lot of SLOC. I think it would be harder to read the program flow, too. Do you have a good example of a data-driven ATF test that does something similar? What I _can_ do, however, is convert some of the macros into functions. That will probably ease debugging. I think I'll go ahead and do that.

[asomers]

Reduce use of macros, and add testcases

[asomers]

Add more testcases.

I have a problem. I really meant to do useful stuff this weekend, but I keep
coming back to tftpd instead. Originally, I thought that I would commit a
small framework that would take care of the weird inetd interaction, with just
a few sample test cases. But now I've gone and written tests that are pretty
much comprehensive except for TFTP option processing.

[asomers]

Fix the build with GCC6

[asomers]

Are you satisfied, @cem ?

[cem]

I am still not a fan of the giant macro approach, but I won't stand in the way of committing more tftpd tests. Go ahead, just don't include me in 'Reviewed by.'