tftpd: Address flaky tests

The tftpd tests all follow the same pattern:
1. open a UDP socket,
2. fork a child to exec tftpd, which subsequently handles requests on
the socket,
3. use a client soc

tftpd: Address flaky tests

The tftpd tests all follow the same pattern:
1. open a UDP socket,
2. fork a child to exec tftpd, which subsequently handles requests on
the socket,
3. use a client socket to send some message to the tftpd daemon.

However, tftpd's first action is to mark its socket as non-blocking and
then read a request from it. If no data is present in the socket, tftpd
exits immediately with an error. So, there is a race; we often see
tftpd test timeouts when running tests in parallel. These timeouts also
arise periodically in CI runs.

One solution is to restructure each test to create the server socket,
then write the request to the client socket, then fork tftpd. This
closes the race. However, this involves a lot of churn.

This patch fixes the problem a different way, by adding a new -b flag to
tftpd which makes it block to read the initial request. Each test is
modified to use -b, closing the race.

Reviewed by: imp, asomers
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D47404

show more ...

tftpd: Code cleanup.

MFC after: 3 days
Sponsored by: Klara, Inc.
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D45871

tftpd: silence gcc overflow warnings

GCC 13 complains that we might be writing too much to an on-stack buffer
when createing a filename.

In practice there is a check that filename isn't too long gi

tftpd: silence gcc overflow warnings

GCC 13 complains that we might be writing too much to an on-stack buffer
when createing a filename.

In practice there is a check that filename isn't too long given the
time format and other static characters so GCC is incorrect, but GCC
isn't wrong that we're potentially trying to put a MAXPATHLEN length
string + some other characters into a MAXPATHLEN buffer (if you ignore
the check GCC can't realistically evaluate at compile time).

Switch to snprintf to populate filename to ensure that future logic
errors don't result in a stack overflow.

Shorten the questionably named yyyymmdd buffer enough to slience the
warning (checking the snprintf return value isn't sufficent) while
preserving maximum flexibility for admins who use the -F option.

MFC after: 3 days
Sponsored by: Klara, Inc.
Reviewed by: brooks
Differential Revision: https://reviews.freebsd.org/D45086

show more ...

tftpd: Satisfy clang-analyzer.

* Replace `random()` with `arc4random()`.
* Change some variable types.
* Drop some unused assignments.

MFC after: 3 days
Sponsored by: Klara, Inc.
Reviewed by: imp,

tftpd: Satisfy clang-analyzer.

* Replace `random()` with `arc4random()`.
* Change some variable types.
* Drop some unused assignments.

MFC after: 3 days
Sponsored by: Klara, Inc.
Reviewed by: imp, markj
Differential Revision: https://reviews.freebsd.org/D45132

show more ...

tftpd: Untangle a conditional.

MFC after: 1 week
Sponsored by: Klara, Inc.
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D45026

tftpd: Immediately reject any request shorter than 4 bytes.

MFC after: 1 week
Sponsored by: Klara, Inc.
Reviewed by: kevans
Differential Revision: https://reviews.freebsd.org/D44957

tftpd: Use `size_t` where appropriate.

* Limit the use of `ssize_t` to only where it's needed.
* Correct one case of `int` being used for a length.

MFC after: 1 week
Sponsored by: Klara, Inc.
Revie

tftpd: Use `size_t` where appropriate.

* Limit the use of `ssize_t` to only where it's needed.
* Correct one case of `int` being used for a length.

MFC after: 1 week
Sponsored by: Klara, Inc.
Reviewed by: kevans
Differential Revision: https://reviews.freebsd.org/D44954

show more ...

Remove copyright strings ifdef'd out

We've ifdef'd out the copyright strings for some time now. Go ahead and
remove the ifdefs. Plus whatever other detritis was left over from other
recent removals.

Remove copyright strings ifdef'd out

We've ifdef'd out the copyright strings for some time now. Go ahead and
remove the ifdefs. Plus whatever other detritis was left over from other
recent removals. These copyright strings are present in the comments and
are largely from CSRG's attempt at adding their copyright to every
binary file (which modern interpretations of the license doesn't
require).

Sponsored by: Netflix

show more ...

libexec: Remove ancient SCCS tags.

Remove ancient SCCS tags from the tree, automated scripting, with two
minor fixup to keep things compiling. All the common forms in the tree
were removed with a pe

libexec: Remove ancient SCCS tags.

Remove ancient SCCS tags from the tree, automated scripting, with two
minor fixup to keep things compiling. All the common forms in the tree
were removed with a perl script.

Sponsored by: Netflix

show more ...

Remove $FreeBSD$: one-line .c pattern

Remove /^[\s*]*__FBSDID\("\$FreeBSD\$"\);?\s*\n/

Revert "tftpd: fix double-colon typo in option string"

It was not a typo: -d takes an optional argument, which is indicated with
a double colon.

This reverts commit 3cbc8e752b92442c784306731e94fd90

Revert "tftpd: fix double-colon typo in option string"

It was not a typo: -d takes an optional argument, which is indicated with
a double colon.

This reverts commit 3cbc8e752b92442c784306731e94fd904de3373c.

Spotted by: kevans

show more ...

tftpd: fix double-colon typo in option string

MFC after: 1 month
X-MFC-With: 273a307d0b80743fb08e23237b3f74dc94a8fa2a
X-MFC-With: 03c2616dc530e5b23f06f9aa421012154590e578

tftpd: unbreak getopt()

Unbreak getopt() broken by recent commit.

Fixes: 273a307d0b80743fb08e23237b3f74dc94a8fa2a
MFC after: 1 month

tftpd: introduce new option -S

Historically, tftpd disallowed write requests to existing files
that are not publicly writable. Such requirement is questionable at least.
Let us make it possible to r

tftpd: introduce new option -S

Historically, tftpd disallowed write requests to existing files
that are not publicly writable. Such requirement is questionable at least.
Let us make it possible to run tftpd in chrooted environment
keeping files non-world writable.

New option -S enables write requests to existing files
for chrooted run according to generic file permissions.
It is ignored unless tftpd runs chrooted.

MFC after: 1 month
Requested by: marck
Differential: https://reviews.freebsd.org/D41090 (based on)

show more ...

tftpd: Don't forget to close stderr on startup.

Just like stdin and stdout, stderr is a copy of the listen socket inherited from inetd. We need to close it so inetd can process further requests, be

tftpd: Don't forget to close stderr on startup.

Just like stdin and stdout, stderr is a copy of the listen socket inherited from inetd. We need to close it so inetd can process further requests, be restarted, etc.

Sponsored by: Klara, Inc.
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D38968

show more ...

tftpd: Don't consume arbitrary requests when failing to fork.

We've already consumed one request, which is sufficient to prevent inetd from endlessly restarting us in this particular and extremely u

tftpd: Don't consume arbitrary requests when failing to fork.

We've already consumed one request, which is sufficient to prevent inetd from endlessly restarting us in this particular and extremely unlikely case.

Sponsored by: Klara, Inc.
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D38967

show more ...

tftpd: Make the -d option behave as documented.

Sponsored by: Klara, Inc.
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D38965

tftpd: whitespace cleanup

tftpd: cleanup

Sponsored by: Klara, Inc.

tftpd: Check for errors from chdir()

MFC after: 1 week
Sponsored by: The FreeBSD Foundation

Fix multiple Coverity warnings in tftpd(8)

* Initialize uninitialized variable (CID 1006502)
* strcpy => strlcpy (CID 1006792, 1006791, 1006790)
* Check function return values (CID 1009442, 1009441,

Fix multiple Coverity warnings in tftpd(8)

* Initialize uninitialized variable (CID 1006502)
* strcpy => strlcpy (CID 1006792, 1006791, 1006790)
* Check function return values (CID 1009442, 1009441, 1009440)
* Delete dead code in receive_packet (not reported by Coverity)
* Remove redundant alarm(3) in receive_packet (not reported by Coverity)

Reported by: Coverity
CID: 1006502, 1006792, 1006791, 1006790, 1009442, 1009441, 1009440
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D11287

show more ...

tftpd: reject unknown opcodes

If tftpd receives a command with an unknown opcode, it simply exits 1. It
doesn't send an ERROR packet, and the client will hang waiting for one. Fix
it.

PR: 226005

tftpd: reject unknown opcodes

If tftpd receives a command with an unknown opcode, it simply exits 1. It
doesn't send an ERROR packet, and the client will hang waiting for one. Fix
it.

PR: 226005
MFC after: 3 weeks

show more ...

tftpd: Abort on an WRQ access violation

On a WRQ (write request) tftpd checks whether the client has access
permission for the file in question. If not, then the write is prevented.
However, tftpd

tftpd: Abort on an WRQ access violation

On a WRQ (write request) tftpd checks whether the client has access
permission for the file in question. If not, then the write is prevented.
However, tftpd doesn't reply with an ERROR packet, nor does it abort.
Instead, it tries to receive the packet anyway.

The symptom is slightly different depending on the nature of the error. If
the target file is nonexistent and tftpd lacks permission to create it, then
tftpd will willingly receive the file, but not write it anywhere. If the
file exists but is not writable, then tftpd will fail to ACK to WRQ.

PR: 225996
MFC after: 3 weeks

show more ...

tftpd: Verify world-writability for WRQ when using relative paths

tftpd(8) says that files may only be written if they already exist and are
publicly writable. tftpd.c verifies that a file is publi

tftpd: Verify world-writability for WRQ when using relative paths

tftpd(8) says that files may only be written if they already exist and are
publicly writable. tftpd.c verifies that a file is publicly writable if it
uses an absolute pathname. However, if the pathname is relative, that check
is skipped. Fix it.

Note that this is not a security vulnerability, because the transfer
ultimately doesn't work unless the file already exists and is owned by user
nobody. Also, this bug does not affect the default configuration, because
the default uses the "-s" option which makes all pathnames absolute.

PR: 226004
MFC after: 3 weeks

show more ...

tftpd: Flush files as soon as they are fully received

On an RRQ, tftpd doesn't exit as soon as it's finished receiving a file.
Instead, it waits five seconds just in case the client didn't receive t

tftpd: Flush files as soon as they are fully received

On an RRQ, tftpd doesn't exit as soon as it's finished receiving a file.
Instead, it waits five seconds just in case the client didn't receive the
server's last ACK and decides to resend the final DATA packet.
Unfortunately, this created a 5 second delay from when the client thinks
it's done sending the file, and when the file is available for other
processes.

Fix this bug by closing the file as soon as receipt is finished.

PR: 157700
Reported by: Barry Mishler <barry_mishler@yahoo.com>
MFC after: 3 weeks

show more ...