Bug 501 - rx does not strip EOF bytes at the end of the last block, leading to incorrect file reception
Summary: rx does not strip EOF bytes at the end of the last block, leading to incorrec...
Status: RESOLVED FIXED
Alias: None
Product: Busybox
Classification: Unclassified
Component: Standard Compliance (show other bugs)
Version: 1.14.x
Hardware: PC Linux
: P5 major
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-29 19:52 UTC by Thomas Petazzoni
Modified: 2009-08-09 21:49 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:


Attachments
Fix (6.59 KB, patch)
2009-08-02 17:39 UTC, Denys Vlasenko
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Petazzoni 2009-07-29 19:52:59 UTC
The xmodem specification states that the last block of data might be padded with 0x1A bytes, up to the 128 bytes block size. And indeed, when sending data using Minicom Xmodem file upload feature, if the file size is not a multiple of 128 bytes, 0x1A are sent.

The issue is that the 'rx' applet doesn't strip those bytes. So the received file on the target is in fact different from the one sent from the host. Its size is rounded up to the next 128 bytes boundary, and this space is padded with 0x1A bytes. Obvisouly the MD5 of the received and sent files do not match.

U-Boot does the following to "detect" such a case, but I'm not sure it's 100% correct :

/* Data blocks can be padded with ^Z (EOF) characters */
/* This code tries to detect and remove them */
if ((xyz.bufp[xyz.len - 1] == EOF) &&
    (xyz.bufp[xyz.len - 2] == EOF) &&
    (xyz.bufp[xyz.len - 3] == EOF))
{
    while (xyz.len
           && (xyz.bufp[xyz.len - 1] == EOF))
    {
           xyz.len--;
    }
}

So I've cooked a patch against the 'rx' applet:

Index: busybox-1.14.2/miscutils/rx.c
===================================================================
--- busybox-1.14.2.orig/miscutils/rx.c
+++ busybox-1.14.2/miscutils/rx.c
@@ -26,6 +26,7 @@
 #define ACK 0x06
 #define NAK 0x15
 #define BS  0x08
+#define PAD 0x1A
 
 /*
 Cf:
@@ -173,6 +174,13 @@
                        goto error;
                }
 
+               for (i = blockLength - 1; i >= 0; i--) {
+                       if (blockBuf[i] == PAD)
+                               blockLength--;
+                       else
+                               break;
+               }
+
                wantBlockNo++;
                length += blockLength;

But this patch is obvisouly wrong, since it removes all the 0x1A at the end of each block. I don't know how to detect:
 1. That we are handling the last block
 2. More importantly, that the 0x1A at the end of the block are actually real data or padding

Unfortunately, due to this bug, the 'rx' applet is currently not usable.
Comment 1 Denys Vlasenko 2009-08-02 17:39:48 UTC
Created attachment 567 [details]
Fix

> I don't know how to detect:
> 1. That we are handling the last block

Easy, it's the last block if you see EOT character after it.

> 2. More importantly, that the 0x1A at the end of the block are actually real
data or padding

Impossible in xmodem protocol, I guess. That's why ymodem and zmodem exist.

Please try this patch