CFDP receive failures caused by incorrect `pdu_data_length` calculation

[davidmayo]

Problem

I believe that the logic for parsing the pdu_data_length field of the CFDP header from raw bytes is incorrect in AIT-DSN in a way that makes receiving files >= 256 bytes fail.

Reproduction steps

1. Replace the file medium.txt in the CFDP datasink/outgoing directory with a file of size 256 bytes

   python -c 'print("+" * 256, end="")' > medium.txt
   

2. Checkout AIT-DSN master
3. Run the ait_cfdp_start_receiver script
4. Run the ait_cfdp_start_sender.py script
5. File transfer will fail.

Discussion

The code for parsing the pdu_data_length field from raw bytes is here. The logic is this:

# left shift first byte 4 to get right position of bits
pdu_data_length = byte_2 << 4
pdu_data_length += byte_3

The part I'm asking about is the left shift. I do not understand why it's there, and it's causing failures in my application.

This logic appears to be unchanged from the very beginning of BLISS/AIT, but I don't understand it. Every version of the CFDP Blue Book (CCSDS CCSDS 727.0-B-5, section 5.1.3) just says that this should be parsed as a UINT16.

This left shift ultimately leads to failure when attempting to receive CFDP files that are larger than >= 256 bytes (i.e., when byte_2 becomes non-zero).

I'm really confused about this 4 bit shift and would like to know why it's there. It's possible I'm just misunderstanding something, and I don't think someone would have coded it this way on accident, but it seems to simply break CFDP receiving as is.

Solution

Changing the code to left shift by 8 instead of 4 makes the logic match the CCSDS spec, and allows for transfers of files larger than 256 bytes*:

# Parse byte_2 and byte_3 as a UINT16
pdu_data_length = byte_2 << 8
pdu_data_length += byte_3

Doing this solves the problem, at least in my application. I would be happy to submit a PR for this, if desired.

*I have found other issues that prevent transferring files larger than 4080 bytes or files with a size in bytes that is not divisible by 8. I intend to submit issues/PR's for those, too.

[davidmayo]

I'm using a minimal config.yaml with nothing special in it:

default:
    dsn:
        cfdp:
            mib:
                path: ./temp/mib
            datasink:
                outgoing:
                    path: ./temp/datasink/outgoing
                incoming:
                    path: ./temp/datasink/incoming
                tempfiles:
                    path: ./temp/datasink/tempfiles
                pdusink:
                    path: ./temp/datasink/pdusink
            max_file_name_length: 64
            max_entity_id_length: 2
            max_transaction_id_length: 4