TFTPD: proposal

Denys Vlasenko vda.linux at googlemail.com
Tue Mar 18 01:12:19 UTC 2008


Hi Vladimir,

AT LAST my ISP managed to turn on cable in my flat.
Sadly, I may move again rather soon... :(
but at least for 2 weeks I should be connected.

I must apologize - I knew that you have your own tftpd
implementation in the wings, but I had no easy way
to sync up. I know that this may feel bad on your side.
Sorry...

On Monday 17 March 2008 21:46, dronnikov at gmail.com wrote:
> + Added 3 options to TFTP:
> -v be verbose -- it used to be uncontrollably _way_ verbose when compiled with DEBUG feature.
> -n N set retry counter -- it used to be hardcoded arbitrary value.
> -t MS set timeout -- it used to be pure arbitrary value which caused
> failures even while using loopback network!

Looks ok.

> - Getting files from current TFTPD is broken.
> Moreover, the simpler are tftp[d]_main() the more complex (and bloaty) is tftp_protocol().

Yes, tftp_protocol() is complex but I don't see what I can remove.
Your patch doesn't touch it. gcc inlines it, though, since now there is
only one callsite. I get:
tftp_main                                            301    1396   +1095
tftp_protocol                                       1173       -   -1173

> My version is smaller and IMO clearer: personally I lost the path of the flow of control 
> in tftp_protocol(). And why all these recvmsg()s when it is updsvd who is responsible for 
> providing plain stdin/out.

Neither udpsvd nor inetd can provide properly bound (bind()ed?) socket.
This a sad limitation of socket API - basically, socket which is already bound
cannot be bound again. See big comment in tcpudp.c (grep for "Doesn't work:").

If you will just use read/write, socket will possibly stay bound to 0.0.0.0:69.
Yes, our inetd and udpsvd will connect() it to peer, so you will get this:

bound to: 0.0.0.0:69  <->  connected to 1.2.3.4:5678

but if local host is multihomed, you will get this situation (taken from dnsd.c):

                /* Try to get *DEST* address (to which of our addresses
                 * this query was directed), and reply from the same address.
                 * Or else we can exhibit usual UDP ugliness:
                 * ip1.multihomed.ip2 <=  query to ip1  <= peer
                 * ip1.multihomed.ip2 => reply from ip2 => peer (confused) */

Why? because if socket is not bound to ip1 but to 0.0.0.0,
kernel has no means how to correctly guess which local IP to use
for outgoing packets! So it uses IP of outgoing interface,
which can be wrong!

There are two solutions:
- for one-packet protocols like DNS, use recv_from_to in order to know
  dst IP on incoming UDP packet, and use this same IP in send_to_from.
- for quasi-connection-like protocols like TFTP, get 1st packet with
  recv_from_to, then create new socket, bind() and connect() it,
  and all future I/O for this connection with this peer should be done
  on this new socket.

> TFTPD is a simple filter implementing a protocol,  
> not UDP-aware particular network utility!

Sad reality prevents this... If only someone will patch kernel to
allow bind() to "move" socket from one address to another,
at least for DGRAM sockets... then udpsvd and inetd can do that
and tftpd can lose all UDP knowledge.

> Next. IMO, access should be controlled by system means: updsvd can set effective user -- 
> its rights should be used when granting (or not) access to files.
> Generic test on "world read/writeness" is way simple but dumb (or too rough). Let us create 
> a specific user and we are done.

Makes sense.

> Please, do consider applying!

Since your patch has all things combined and I want to keep existing UDP voodoo magic,
it's a bit hard to apply parts that I like. I will try to manually do that...


                        s += strlen(s)+1;

You can run off the packet here.

                        s = tftp_option_get(buf+2, len-2, "blksize");
                        if (s) {
                                // reallocate transfer buffer
                                blksize = xatoi_u(s);

If someone will try to up/download a file called "blksize",
xatoi_u() will be fed with string "octet". Not good.
--
vda



More information about the busybox mailing list