[net] TFTPClient's send file discards last ack

• Key: NET-68
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Perttu Auramo
• Affects Version/s: 1.3
• Fix Version/s: 2.0, 1.5

Environment

Operating System: Linux
Platform: PC

Description

TFTPClient reads all acks just fine except the last-one when sending a file. I
figured this out when I tried to use the same TFTPClient-instance for something
else (reading a file) after sending a file. This ack was next in the buffer and
some exception was thrown (don't remember which anymore).

I fixed this for myself using a flag (lastAckWait). Here is a the result of
diff-command:

diff -u TFTPClient.java.original TFTPClient.java.patched
— TFTPClient.java.original 2004-12-28 15:02:37.235997984 +0200
+++ TFTPClient.java.patched 2004-12-28 15:09:14.516602152 +0200
@@ -372,6 +372,7 @@

dataLength = lastBlock = hostPort = bytesRead = 0;
block = 0;
+ boolean lastAckWait = false;

if (mode == TFTP.ASCII_MODE)
input = new ToNetASCIIInputStream(input);
@@ -455,7 +456,10 @@
if (lastBlock == block)

{ ++block; - break _receivePacket; + if (lastAckWait) + break _sendPacket; + else + break _receivePacket; }

else

{ @@ -501,9 +505,8 @@ data.setData(_sendBuffer, 4, offset - 4); sent = data; }

• while (dataLength == 0);
  + while (dataLength == 0 || lastAckWait);

• bufferedSend(sent);
  endBufferedOps();
  }

By the way we have implemented a TFTP server also (heavily unit-tested). I could
try to contribute it back if it fits in commons net. There was some talk in the
web-pages of doing only client-side stuff for commons-net.

-Perttu

Comments

[Perttu Auramo]

Created an attachment (id=13845)
The patch as a file

[Rory Winston]

Thanks Perttu.

[michael.enright]

I can't see how the flag in this patch helps anything. As far as I can tell, all
the lines in the patch that could make use of this new flag have + in front of
them. None of those lines set the flag to true, so the flag is always false and
the code pretty much does what it did before.

There is another delta in the patch that changes how bufferedSend is called.
This worries me because I'm having a problem where the last piece of the file
(or all of the file if it is small) is not sent to the server.

[michael.enright]

FWIW restoring the deleted call to bufferedSend corrected the symptoms I had.

[Daniel Savarese]

I concur with your assessment Michael. I don't see what this patch does
other than break TFTPClient.sendFile. Even if lastAckWait were true, the
method would go into an infinite loop at the end. I'm going to revert the
change.

If the original issue reporter believes there is a bug in sendFile, then
we're going to need a test program that reproduces the bug. Submitting
a patch with only anectdotal support for there being a problem is
insufficient.

[Daniel Savarese]

It is true that sendFile doesn't wait for a final ack (probably because
it was thought at the time that we didn't want to hang if the final ack
got lost). Therefore, a UDP packet may remain queued for delivery to
user space, causing a problem on subsequent read operation on the same
socket. However, a call to TFTP.discardPackets is the appropriate
(although imperfect if the last ack is delayed) workaround until we get
more information about the problem.

[Daniel Savarese]

• • • COM-2188 has been marked as a duplicate of this bug. ***

[Rory Winston]

Daniel has reverted the fix.

[Jennifer Hodgdon]

Neither the 1.41 release version of TFTPClient.java, nor the current nightly build version (as of Sept 15 2006) is working, actually. This bug is still happening. I will attach a zip file (which I just mistakenly put into Bugzilla, sorry!) with some test code.

The nightly build version is definitely doing what the bug report stated: dropping the last ACK. When TFTPClient sends a file, it sends a data packet and waits for an ACK, but after the last data packet, it doesn't pick up the last ACK, and then when TFTPClient goes to send/receive another file from the same server, the first thing it gets is the last ACK from the previous send, and it reports an "unexpected packet received" and fails.

The version from the 1.41 release doesn't work either – I can't recall exactly what is wrong with it, actually, but I tested it and it does not work.

The version I will attach does work. It would probably be a good idea to use something like this in the next release.

I'll also attach my test code: a TFTPClientApp and TFTPServerApp that can be run on two different computers to test the workings of the TFTPClient class.

This zip file contains a fix for TFTPClient.java, as well as TFTP test code. TFTPClientApp and TFTPServerApp need to be run on two separate computers – see embedded doc for more information.

[Jennifer Hodgdon]

Patch for suggested fix to TFTPClient.java

[Rory Winston]

I've applied your patch to TFTPClient on the 2.0 branch. Thanks Jennifer.

[Dan Armbrust]

Why hasn't this patch been applied to 1.4? What good does it do fixing in in 2.0, if that never gets built?

The 1.4 release, currently available for download is quite simply, broken. Files smaller than 1 block size simply aren't transferred, larger files are corrupted.