[ECOS] RedBoot xyModem bug

[ECOS] RedBoot xyModem bug

[Rutger Hofman]

Hello list,

I ran into a bug in RedBoot xyModem.c. It is in lines 420..434 in the 
current version (cvs 1.21):

     while (!xyz.at_eof && (size > 0)) {
... code for each block in the xyzModem protocols: ...

#if defined(xyzModem_zmodem) || defined(USE_YMODEM_LENGTH)
                         if (xyz.mode == xyzModem_xmodem || 
xyz.file_length == 0) {
#else
                         if (1) {
#endif
                             // 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--;
                                 }
                             }
                         }

What this does, is inspect each uploaded block for trailing ^Z = 0x1a 
bytes, and strip them. Rationale: xmodem or ymodem may signal 
end-of-file with ^Z a.k.a. 'CP/M EOF', and may optionally pad the block 
with more ^Z chars. This is live behaviour: sx and sy (0.12.20) under 
Linux do ^Z padding in the last block. The xyModem.c code above is 
wrong. ^Z chars are stripped at the end of *each block* in stead of at 
the end of the file.

This problem surfaced when I enabled zlib in eCos. In one of its tables, 
there is a sequence of bytes 26 = 0x1a = ^Z (in 
packages/services/compress/zlib/current/src/trees.h, lines 90, 91, 
112-114), which may trigger the test above. Depending on the offset in 
the file, these 0x1a's hit a block boundary or not, and the file is 
telescope-truncated or not. (I noticed that my executable crashed or not 
depending on code size, even depending on a number of 'nop' assembly 
instructions that I inserted anywhere in the code; it was a beast of a 
bug to pin down...)

The fix is nonobvious to me. For ymodem, if #define USE_YMODEM_LENGTH is 
on and a file length is specified, any trailing characters are removed 
elsewhere: this works correctly. However, for xmodem or ymodem without 
file length, any trailing ^Z at the end-of-file must still be removed. 
The code above is inherently unaware whether this is the last block or 
not: xmodem sends end-of-file control info, but that is consumed only in 
the *next* block read. The solution might be to maintain one more 
read-ahead block buffer to see if the current block is the last normal 
block.

Some random remarks:

As a workaround (I dare not flash the RedBoot in my device, because we 
bought it ready-made; I use the code base for my eCos application), I 
tried disabling the table in zlib. That lead to another problem, which I 
will report separately.

I am in the dark what must be done if a file to be xmodem'd actually 
contains trailing ^Z characters. There is nothing that forbids that 
outside CP/M or MS-DOS, I'd say.

I also wondered how this bug can have lived for so long in RedBoot. Or 
doesn't anyone use xyModem nowadays?

Rutger Hofman
VU Amsterdam

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] RedBoot xyModem bug

[Rutger Hofman]

Is *really* *nobody* interested in bugs in the RedBoot upload 
implementation?

Rutger

Rutger Hofman wrote on Jan 11, 2008:
> Hello list,
> 
> I ran into a bug in RedBoot xyModem.c. It is in lines 420..434 in the 
> current version (cvs 1.21):
> 
>     while (!xyz.at_eof && (size > 0)) {
> ... code for each block in the xyzModem protocols: ...
> 
> #if defined(xyzModem_zmodem) || defined(USE_YMODEM_LENGTH)
>                         if (xyz.mode == xyzModem_xmodem || 
> xyz.file_length == 0) {
> #else
>                         if (1) {
> #endif
>                             // 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--;
>                                 }
>                             }
>                         }
> 
> What this does, is inspect each uploaded block for trailing ^Z = 0x1a 
> bytes, and strip them. Rationale: xmodem or ymodem may signal 
> end-of-file with ^Z a.k.a. 'CP/M EOF', and may optionally pad the block 
> with more ^Z chars. This is live behaviour: sx and sy (0.12.20) under 
> Linux do ^Z padding in the last block. The xyModem.c code above is 
> wrong. ^Z chars are stripped at the end of *each block* in stead of at 
> the end of the file.
> 
> This problem surfaced when I enabled zlib in eCos. In one of its tables, 
> there is a sequence of bytes 26 = 0x1a = ^Z (in 
> packages/services/compress/zlib/current/src/trees.h, lines 90, 91, 
> 112-114), which may trigger the test above. Depending on the offset in 
> the file, these 0x1a's hit a block boundary or not, and the file is 
> telescope-truncated or not. (I noticed that my executable crashed or not 
> depending on code size, even depending on a number of 'nop' assembly 
> instructions that I inserted anywhere in the code; it was a beast of a 
> bug to pin down...)
> 
> The fix is nonobvious to me. For ymodem, if #define USE_YMODEM_LENGTH is 
> on and a file length is specified, any trailing characters are removed 
> elsewhere: this works correctly. However, for xmodem or ymodem without 
> file length, any trailing ^Z at the end-of-file must still be removed. 
> The code above is inherently unaware whether this is the last block or 
> not: xmodem sends end-of-file control info, but that is consumed only in 
> the *next* block read. The solution might be to maintain one more 
> read-ahead block buffer to see if the current block is the last normal 
> block.
> 
> Some random remarks:
> 
> As a workaround (I dare not flash the RedBoot in my device, because we 
> bought it ready-made; I use the code base for my eCos application), I 
> tried disabling the table in zlib. That lead to another problem, which I 
> will report separately.
> 
> I am in the dark what must be done if a file to be xmodem'd actually 
> contains trailing ^Z characters. There is nothing that forbids that 
> outside CP/M or MS-DOS, I'd say.
> 
> I also wondered how this bug can have lived for so long in RedBoot. Or 
> doesn't anyone use xyModem nowadays?
> 
> Rutger Hofman
> VU Amsterdam
> 


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] RedBoot xyModem bug

[Gary Thomas]

Rutger Hofman wrote:
> Is *really* *nobody* interested in bugs in the RedBoot upload 
> implementation?
> 
> Rutger
> 
> Rutger Hofman wrote on Jan 11, 2008:
>> Hello list,
>>
>> I ran into a bug in RedBoot xyModem.c. It is in lines 420..434 in the 
>> current version (cvs 1.21):
>>
>>     while (!xyz.at_eof && (size > 0)) {
>> ... code for each block in the xyzModem protocols: ...
>>
>> #if defined(xyzModem_zmodem) || defined(USE_YMODEM_LENGTH)
>>                         if (xyz.mode == xyzModem_xmodem || 
>> xyz.file_length == 0) {
>> #else
>>                         if (1) {
>> #endif
>>                             // 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--;
>>                                 }
>>                             }
>>                         }
>>
>> What this does, is inspect each uploaded block for trailing ^Z = 0x1a 
>> bytes, and strip them. Rationale: xmodem or ymodem may signal 
>> end-of-file with ^Z a.k.a. 'CP/M EOF', and may optionally pad the 
>> block with more ^Z chars. This is live behaviour: sx and sy (0.12.20) 
>> under Linux do ^Z padding in the last block. The xyModem.c code above 
>> is wrong. ^Z chars are stripped at the end of *each block* in stead of 
>> at the end of the file.
>>
>> This problem surfaced when I enabled zlib in eCos. In one of its 
>> tables, there is a sequence of bytes 26 = 0x1a = ^Z (in 
>> packages/services/compress/zlib/current/src/trees.h, lines 90, 91, 
>> 112-114), which may trigger the test above. Depending on the offset in 
>> the file, these 0x1a's hit a block boundary or not, and the file is 
>> telescope-truncated or not. (I noticed that my executable crashed or 
>> not depending on code size, even depending on a number of 'nop' 
>> assembly instructions that I inserted anywhere in the code; it was a 
>> beast of a bug to pin down...)
>>
>> The fix is nonobvious to me. For ymodem, if #define USE_YMODEM_LENGTH 
>> is on and a file length is specified, any trailing characters are 
>> removed elsewhere: this works correctly. However, for xmodem or ymodem 
>> without file length, any trailing ^Z at the end-of-file must still be 
>> removed. The code above is inherently unaware whether this is the last 
>> block or not: xmodem sends end-of-file control info, but that is 
>> consumed only in the *next* block read. The solution might be to 
>> maintain one more read-ahead block buffer to see if the current block 
>> is the last normal block.
>>
>> Some random remarks:
>>
>> As a workaround (I dare not flash the RedBoot in my device, because we 
>> bought it ready-made; I use the code base for my eCos application), I 
>> tried disabling the table in zlib. That lead to another problem, which 
>> I will report separately.

Why not test using a RAM RedBoot configuration?  This should be
totally safe and won't "brick" your device.

>> I am in the dark what must be done if a file to be xmodem'd actually 
>> contains trailing ^Z characters. There is nothing that forbids that 
>> outside CP/M or MS-DOS, I'd say.
>>
>> I also wondered how this bug can have lived for so long in RedBoot. Or 
>> doesn't anyone use xyModem nowadays?

Indeed, it does get used, but this problem has only been
seen/reported by you.  If you want to see things happen, send patches.

-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] RedBoot xyModem bug

[Jürgen Lambrecht]

>>
>> I also wondered how this bug can have lived for so long in RedBoot. 
>> Or doesn't anyone use xyModem nowadays?
>>
I used xyModem in the beginning, but it only worked for 19600 baud, and 
not for higher baud rates. Also others have had problems with xyModem.
So I wrote my Ethernet driver and started using tftp instead...

Regards,
Jürgen

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss