On Wed, May 20, 2015 at 4:04 PM, Linus Torvalds
<[email protected]> wrote:
>
> I suspect that my "readdir()" thing has the potential for the same
> issue, and unlike "read_file()" I don't know ahead of time exactly how
> many bytes I'm supposed to get.
>
> I think I know what the protocol expects me to do (I should look at
> the first reply packet to see the final size - right now I just
> _verify_ the final size against it), but that will be a larger patch.
>
> I'll think some more about this, but expect another patch later today.
> The patch I sent should be fine for hopefully fixing the case that hit
> Guenther, so this "more patches to come" is more about future-proofing
> than anything else.
Ok, this is the more complete patch. It goes on top of the previous
one, although I guess technically it really ends up undoing it
entirely because we just do the reading differently.
It ended up being a bit bigger than I hoped for, but I think the end
result is good. It's *much* more explicit and careful about sizes of
the replies from the dive computer and there is no more "read more
packets until you can get no more" kind of logic, we now use the
expected size and read that many bytes. That's clearly (in hind-sight)
how the protocol was designed - my original "partial data packet marks
the end" model came from how I noticed the big patters before noticing
the specific details in the headers of the reply sequence.
Again, "it works for me". Clearly more testing would be good, but
since this tightens up the logic, if I had gotten things very wrong,
it would be very obvious. So I think it should be good.
Linus
From 60c6eca21f2fcd919f1f224ea3602a93bcea9f62 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <[email protected]>
Date: Thu, 21 May 2015 11:02:11 -0700
Subject: [PATCH 2/2] 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 <[email protected]>
---
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 35249031a7dd..5713aa43d9d5 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;
-
- while (size > 0) {
- int rc, transferred, len;
+ const int InEndpoint = 0x82;
+ 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;
--
2.4.0.53.g8440f74
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
