Skip to content

io_legacy Comm::tx vs. Comm::tx_length status byte handling is inconsistent #431

Description

@ryan-sentz

Comm::reply extends the io_buffer with the status bytes before calling Comm::apdu_send

pub fn reply<T: Into<Reply>>(&mut self, reply: T) {
let sw = reply.into().0;
// Append status word
self.io_buffer[self.tx_length] = (sw >> 8) as u8;
self.io_buffer[self.tx_length + 1] = sw as u8;
self.tx_length += 2;
// Transmit the response
self.apdu_send();
}

Comm::apdu_send now has a switch to detect whether the old apdu_buffer (with tx) or new io_buffer (with tx_length) are being used

if self.tx != 0 {
sys_seph::io_tx(self.apdu_type, &self.apdu_buffer, self.tx);
self.tx = 0;
} else {
sys_seph::io_tx(self.apdu_type, &self.io_buffer, self.tx_length);
}
self.tx_length = 0;
self.rx_length = 0;
// Replying completes the current command.
self.apdu_in_progress = false;

This means that existing code that sets apdu_buffer and tx no longer correctly appends status bytes for messages containing data.

The fix from an application side is pretty straightforward, just swapping to io_buffer and tx_length, but, I believe it's a breaking (and unexpected) change in behaviour so it seemed worth a report.

(As an aside, I appreciate that the buffer is still externally accessible as we encode directly into it to reduce memory use. Also it'd be nice to have docstrings on the Comm fields so you don't have to dig through the code to work out which to use.)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions