From e73dcdacaeb5fc4db3bf8b837ff9476e2dcf18cc Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Thu, 21 May 2015 11:02:11 -0700 Subject: [PATCH] suunto eon steel: be more explicit about transfer sizes When reading data from the EON Steel, we'd generally continue reading until we saw that a response was done by seeing a packet that wasn't full. That broke for the case of the data boundary matching the packet boundary, fixed by the commit "suunto eon steel: fix file reading special case". However, that commit only fixed it for the case of reading a file, where the result has a size that is known up-front. And most other situations really don't matter, because the result size is fixed and fits in a single packet, so it all works. However, there are still a few cases that could trigger the problem, notably reading the directory contents. So change the send_receive() logic to actually read the expected size from the receive header in the first packet of the reply. This means that we need to re-organize the packet reception code a bit, but the end result is that we are much more careful about data sizes, This also changes the packet logging to be much more readable, by logging just the actual data, and not the (uninteresting) per-packet header, or the stale data at the end of the packet. Signed-off-by: Linus Torvalds Signed-off-by: Dirk Hohndel --- src/suunto_eonsteel.c | 174 +++++++++++++++++++++++++++++------------- 1 file changed, 120 insertions(+), 54 deletions(-) diff --git a/src/suunto_eonsteel.c b/src/suunto_eonsteel.c index 3524903..5713aa4 100644 --- a/src/suunto_eonsteel.c +++ b/src/suunto_eonsteel.c @@ -125,43 +125,45 @@ static void put_le32(unsigned int val, unsigned char *p) p[3] = val >> 24; } -static int receive_data(suunto_eonsteel_device_t *eon, unsigned char *buffer, int size) +/* + * Get a single 64-byte packet from the dive computer. This handles packet + * logging and any obvious packet-level errors, and returns the payload of + * packet. + * + * The two first bytes of the packet are packet-level metadata: the report + * type (always 0x3f), and then the size of the valid data in the packet. + * + * The maximum payload is 62 bytes. + */ +#define PACKET_SIZE 64 +static int receive_packet(suunto_eonsteel_device_t *eon, unsigned char *buffer, int size) { - const int InEndpoint = 0x82; unsigned char buf[64]; - int ret = 0; + const int InEndpoint = 0x82; + int rc, transferred, len; - while (size > 0) { - int rc, transferred, len; - - rc = libusb_interrupt_transfer(eon->handle, InEndpoint, buf, sizeof(buf), &transferred, 5000); - if (rc || transferred != sizeof(buf)) { - ERROR(eon->base.context, "incomplete read interrupt transfer"); - return -1; - } - // dump every incoming packet? - HEXDUMP (eon->base.context, DC_LOGLEVEL_DEBUG, "rcv", buf, transferred); - if (buf[0] != 0x3f) { - ERROR(eon->base.context, "read interrupt transfer returns wrong report type"); - return -1; - } - len = buf[1]; - if (len > sizeof(buf)-2) { - ERROR(eon->base.context, "read interrupt transfer reports short length"); - return -1; - } - if (len > size) { - ERROR(eon->base.context, "read interrupt transfer reports excessive length"); - return -1; - } - memcpy(buffer+ret, buf+2, len); - size -= len; - ret += len; - if (len < sizeof(buf)-2) - break; + /* 5000 = 5s timeout */ + rc = libusb_interrupt_transfer(eon->handle, InEndpoint, buf, PACKET_SIZE, &transferred, 5000); + if (rc || transferred != PACKET_SIZE) { + ERROR(eon->base.context, "incomplete read interrupt transfer"); + return -1; } - - return ret; + if (buf[0] != 0x3f) { + ERROR(eon->base.context, "read interrupt transfer returns wrong report type (%d)", buf[0]); + return -1; + } + len = buf[1]; + if (len > PACKET_SIZE-2) { + ERROR(eon->base.context, "read interrupt transfer reports bad length (%d)", len); + return -1; + } + if (len > size) { + ERROR(eon->base.context, "receive_packet result buffer too small - truncating"); + len = size; + } + HEXDUMP (eon->base.context, DC_LOGLEVEL_DEBUG, "rcv", buf+2, len); + memcpy(buffer, buf+2, len); + return len; } static int send_cmd(suunto_eonsteel_device_t *eon, @@ -210,10 +212,67 @@ static int send_cmd(suunto_eonsteel_device_t *eon, } // dump every outgoing packet? - HEXDUMP (eon->base.context, DC_LOGLEVEL_DEBUG, "cmd", buf, sizeof(buf)); + HEXDUMP (eon->base.context, DC_LOGLEVEL_DEBUG, "cmd", buf+2, len+12); return 0; } +struct eon_hdr { + unsigned short cmd; + unsigned int magic; + unsigned short seq; + unsigned int len; +}; + +static int receive_header(suunto_eonsteel_device_t *eon, struct eon_hdr *hdr, unsigned char *buffer, int size) +{ + int ret; + unsigned char header[64]; + + ret = receive_packet(eon, header, sizeof(header)); + if (ret < 0) + return -1; + if (ret < 12) { + ERROR(eon->base.context, "short reply packet (%d)", ret); + return -1; + } + + /* Unpack the 12-byte header */ + hdr->cmd = array_uint16_le(header); + hdr->magic = array_uint32_le(header+2); + hdr->seq = array_uint16_le(header+6); + hdr->len = array_uint32_le(header+8); + + ret -= 12; + if (ret > size) { + ERROR(eon->base.context, "receive_header result data buffer too small (%d vs %d)", ret, size); + return -1; + } + memcpy(buffer, header+12, ret); + return ret; +} + +static int receive_data(suunto_eonsteel_device_t *eon, unsigned char *buffer, int size) +{ + int ret = 0; + + while (size > 0) { + int len; + + len = receive_packet(eon, buffer + ret, size); + if (len < 0) + return -1; + + size -= len; + ret += len; + + /* Was it not a full packet of data? We're done, regardless of expectations */ + if (len < PACKET_SIZE-2) + break; + } + + return ret; +} + /* * Send a command, receive a reply * @@ -235,43 +294,49 @@ static int send_receive(suunto_eonsteel_device_t *eon, { int len, actual, max; unsigned char buf[2048]; + struct eon_hdr hdr; if (send_cmd(eon, cmd, len_out, out) < 0) return -1; - max = len_in + 12; - if (max > sizeof(buf)) - max = sizeof(buf); - len = receive_data(eon, buf, max); - if (len < 10) { - ERROR(eon->base.context, "short command reply (%d)", len); + + /* Get the header and the first part of the data */ + len = receive_header(eon, &hdr, in, len_in); + if (len < 0) return -1; - } - if (array_uint16_le(buf) != cmd) { + + /* Verify the header data */ + if (hdr.cmd != cmd) { ERROR(eon->base.context, "command reply doesn't match command"); return -1; } - if (array_uint32_le(buf+2) != eon->magic + 5) { - ERROR(eon->base.context, "command reply doesn't match magic (got %08x, expected %08x)", array_uint32_le(buf+2), eon->magic + 5); + if (hdr.magic != eon->magic + 5) { + ERROR(eon->base.context, "command reply doesn't match magic (got %08x, expected %08x)", hdr.magic, eon->magic + 5); return -1; } - if (array_uint16_le(buf+6) != eon->seq) { + if (hdr.seq != eon->seq) { ERROR(eon->base.context, "command reply doesn't match sequence number"); return -1; } - actual = array_uint32_le(buf+8); - if (actual + 12 != len) { - ERROR(eon->base.context, "command reply length mismatch (got %d, claimed %d)", len-12, actual); + actual = hdr.len; + if (actual < len) { + ERROR(eon->base.context, "command reply length mismatch (got %d, claimed %d)", len, actual); return -1; } - if (len_in < actual) { - ERROR(eon->base.context, "command reply returned too much data (got %d, had %d)", actual, len_in); + if (actual > len_in) { + ERROR(eon->base.context, "command reply too big for result buffer - truncating"); + actual = len_in; + } + + /* Get the rest of the data */ + len += receive_data(eon, in + len, actual - len); + if (len != actual) { + ERROR(eon->base.context, "command reply returned unexpected amoutn of data (got %d, expected %d)", len, actual); return -1; } // Successful command - increment sequence number eon->seq++; - memcpy(in, buf+12, actual); - return actual; + return len; } static int read_file(suunto_eonsteel_device_t *eon, const char *filename, dc_buffer_t *buf) @@ -455,6 +520,7 @@ static int initialize_eonsteel(suunto_eonsteel_device_t *eon) const int InEndpoint = 0x82; const unsigned char init[] = {0x02, 0x00, 0x2a, 0x00}; unsigned char buf[64]; + struct eon_hdr hdr; /* Get rid of any pending stale input first */ for (;;) { @@ -471,13 +537,13 @@ static int initialize_eonsteel(suunto_eonsteel_device_t *eon) ERROR(eon->base.context, "Failed to send initialization command"); return -1; } - if (receive_data(eon, buf, sizeof(buf)) < 0) { + if (receive_header(eon, &hdr, buf, sizeof(buf)) < 0) { ERROR(eon->base.context, "Failed to receive initial reply"); return -1; } // Don't ask - eon->magic = 0x00000005 | (buf[4] << 16) | (buf[5] << 24); + eon->magic = (hdr.magic & 0xffff0000) | 0x0005; // Increment the sequence number for every command sent eon->seq++; return 0;