From e0cf41a14e730a3c83e9c0d4ff548b029f7e55f8 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 11 Dec 2023 21:02:03 +0100 Subject: [PATCH 1/3] Add some extra parameter validation The ringbuffer boundary addresses (begin/end) should be ordered correctly, and the packet size should be smaller than the ringbuffer size, otherwise the code won't work as expected. --- src/rbstream.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/rbstream.c b/src/rbstream.c index 07188e0..e16f872 100644 --- a/src/rbstream.c +++ b/src/rbstream.c @@ -78,6 +78,18 @@ dc_rbstream_new (dc_rbstream_t **out, dc_device_t *device, unsigned int pagesize return DC_STATUS_INVALIDARGS; } + // Ringbuffer boundaries should not be reversed. + if (begin > end) { + ERROR (device->context, "Ringbuffer boundaries reversed!"); + return DC_STATUS_INVALIDARGS; + } + + // Packet size should be smaller than the ringbuffer size. + if (packetsize > (end - begin)) { + ERROR (device->context, "Packet size larger than the ringbuffer size!"); + return DC_STATUS_INVALIDARGS; + } + // Address should be inside the ringbuffer. if (address < begin || address > end) { ERROR (device->context, "Address outside the ringbuffer!"); From 00b016957822cef5ce96f3d4e48cf4e447e2b6f7 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 11 Dec 2023 21:25:54 +0100 Subject: [PATCH 2/3] Update the internal state in-place Some of the internal state is cached in local variables at the start of the function, and is updated only at the end of the function. But the contents of the packet buffer is never cached. As a result, the two can go out of sync when an error occurs and the function returns early. Trying to restore the original state is pointless if the corresponding data in the packet buffer is no longer available. Fixed by removing the local variables and always updating the internal state in-place to reflect the current state. --- src/rbstream.c | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/src/rbstream.c b/src/rbstream.c index e16f872..8e7e17a 100644 --- a/src/rbstream.c +++ b/src/rbstream.c @@ -125,43 +125,39 @@ dc_rbstream_read (dc_rbstream_t *rbstream, dc_event_progress_t *progress, unsign if (rbstream == NULL) return DC_STATUS_INVALIDARGS; - unsigned int address = rbstream->address; - unsigned int available = rbstream->available; - unsigned int skip = rbstream->skip; - unsigned int nbytes = 0; unsigned int offset = size; while (nbytes < size) { - if (available == 0) { + if (rbstream->available == 0) { // Handle the ringbuffer wrap point. - if (address == rbstream->begin) - address = rbstream->end; + if (rbstream->address == rbstream->begin) + rbstream->address = rbstream->end; // Calculate the packet size. unsigned int len = rbstream->packetsize; - if (rbstream->begin + len > address) - len = address - rbstream->begin; - - // Move to the begin of the current packet. - address -= len; + if (rbstream->begin + len > rbstream->address) + len = rbstream->address - rbstream->begin; // Read the packet into the cache. - rc = dc_device_read (rbstream->device, address, rbstream->cache, rbstream->packetsize); + rc = dc_device_read (rbstream->device, rbstream->address - len, rbstream->cache, rbstream->packetsize); if (rc != DC_STATUS_SUCCESS) return rc; - available = len - skip; - skip = 0; + // Move to the end of the next packet. + rbstream->address -= len; + + rbstream->available = len - rbstream->skip; + rbstream->skip = 0; } - unsigned int length = available; + unsigned int length = rbstream->available; if (nbytes + length > size) length = size - nbytes; offset -= length; - available -= length; + rbstream->available -= length; - memcpy (data + offset, rbstream->cache + available, length); + memcpy (data + offset, rbstream->cache + rbstream->available, length); // Update and emit a progress event. if (progress) { @@ -172,10 +168,6 @@ dc_rbstream_read (dc_rbstream_t *rbstream, dc_event_progress_t *progress, unsign nbytes += length; } - rbstream->address = address; - rbstream->available = available; - rbstream->skip = skip; - return rc; } From 9090f713b46b585532f7c89f1c250eb30a178795 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Fri, 17 Nov 2023 19:49:42 +0100 Subject: [PATCH 3/3] Add support for reading a ringbuffer forwards A dive computer typically writes its ringbuffer in the forward direction. Thus, when downloading the dives in reverse order (newest first), the ringbuffer needs to be read in the backward direction. However, some dive computers already re-arrange the data in the correct order, which means the data needs to be read in the opposite direction. To support this case without drastic changes in the logic, the rbstream implementation is extended to also support reading in the forward direction. --- src/cochran_commander.c | 2 +- src/cressi_edy.c | 2 +- src/liquivision_lynx.c | 4 +- src/mares_iconhd.c | 2 +- src/oceanic_common.c | 4 +- src/rbstream.c | 89 +++++++++++++++++++++++++++++++++++++---- src/rbstream.h | 11 ++++- src/seac_screen.c | 2 +- src/suunto_common2.c | 2 +- src/zeagle_n2ition3.c | 2 +- 10 files changed, 101 insertions(+), 19 deletions(-) diff --git a/src/cochran_commander.c b/src/cochran_commander.c index 38328ae..2dbdc1b 100644 --- a/src/cochran_commander.c +++ b/src/cochran_commander.c @@ -965,7 +965,7 @@ cochran_commander_device_foreach (dc_device_t *abstract, dc_dive_callback_t call last_start_address = base + array_uint32_le(data.config + layout->cf_last_log ); // Create the ringbuffer stream. - status = dc_rbstream_new (&rbstream, abstract, 1, layout->rbstream_size, layout->rb_profile_begin, layout->rb_profile_end, last_start_address); + status = dc_rbstream_new (&rbstream, abstract, 1, layout->rbstream_size, layout->rb_profile_begin, layout->rb_profile_end, last_start_address, DC_RBSTREAM_BACKWARD); if (status != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); goto error; diff --git a/src/cressi_edy.c b/src/cressi_edy.c index d61b84e..41a9397 100644 --- a/src/cressi_edy.c +++ b/src/cressi_edy.c @@ -481,7 +481,7 @@ cressi_edy_device_foreach (dc_device_t *abstract, dc_dive_callback_t callback, v // Create the ringbuffer stream. dc_rbstream_t *rbstream = NULL; - rc = dc_rbstream_new (&rbstream, abstract, SZ_PAGE, SZ_PACKET, layout->rb_profile_begin, layout->rb_profile_end, eop); + rc = dc_rbstream_new (&rbstream, abstract, SZ_PAGE, SZ_PACKET, layout->rb_profile_begin, layout->rb_profile_end, eop, DC_RBSTREAM_BACKWARD); if (rc != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); return rc; diff --git a/src/liquivision_lynx.c b/src/liquivision_lynx.c index b755955..51423b6 100644 --- a/src/liquivision_lynx.c +++ b/src/liquivision_lynx.c @@ -469,7 +469,7 @@ liquivision_lynx_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb // Create the ringbuffer stream. dc_rbstream_t *rblogbook = NULL; - status = dc_rbstream_new (&rblogbook, abstract, SEGMENTSIZE, SEGMENTSIZE, RB_LOGBOOK_BEGIN, RB_LOGBOOK_END, rb_logbook_end); + status = dc_rbstream_new (&rblogbook, abstract, SEGMENTSIZE, SEGMENTSIZE, RB_LOGBOOK_BEGIN, RB_LOGBOOK_END, rb_logbook_end, DC_RBSTREAM_BACKWARD); if (status != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); goto error_free_logbook; @@ -600,7 +600,7 @@ liquivision_lynx_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb // Create the ringbuffer stream. dc_rbstream_t *rbprofile = NULL; - status = dc_rbstream_new (&rbprofile, abstract, SEGMENTSIZE, SEGMENTSIZE, RB_PROFILE_BEGIN, RB_PROFILE_END, rb_profile_end); + status = dc_rbstream_new (&rbprofile, abstract, SEGMENTSIZE, SEGMENTSIZE, RB_PROFILE_BEGIN, RB_PROFILE_END, rb_profile_end, DC_RBSTREAM_BACKWARD); if (status != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); goto error_free_profile; diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index ede2658..c8dd0b0 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -686,7 +686,7 @@ mares_iconhd_device_foreach_raw (dc_device_t *abstract, dc_dive_callback_t callb // Create the ringbuffer stream. dc_rbstream_t *rbstream = NULL; - rc = dc_rbstream_new (&rbstream, abstract, 1, device->packetsize, layout->rb_profile_begin, layout->rb_profile_end, eop); + rc = dc_rbstream_new (&rbstream, abstract, 1, device->packetsize, layout->rb_profile_begin, layout->rb_profile_end, eop, DC_RBSTREAM_BACKWARD); if (rc != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); return rc; diff --git a/src/oceanic_common.c b/src/oceanic_common.c index 62f5f74..e9b3d32 100644 --- a/src/oceanic_common.c +++ b/src/oceanic_common.c @@ -327,7 +327,7 @@ oceanic_common_device_logbook (dc_device_t *abstract, dc_event_progress_t *progr // Create the ringbuffer stream. dc_rbstream_t *rbstream = NULL; - rc = dc_rbstream_new (&rbstream, abstract, PAGESIZE, PAGESIZE * device->multipage, layout->rb_logbook_begin, layout->rb_logbook_end, rb_logbook_end); + rc = dc_rbstream_new (&rbstream, abstract, PAGESIZE, PAGESIZE * device->multipage, layout->rb_logbook_begin, layout->rb_logbook_end, rb_logbook_end, DC_RBSTREAM_BACKWARD); if (rc != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); return rc; @@ -489,7 +489,7 @@ oceanic_common_device_profile (dc_device_t *abstract, dc_event_progress_t *progr // Create the ringbuffer stream. dc_rbstream_t *rbstream = NULL; - rc = dc_rbstream_new (&rbstream, abstract, PAGESIZE, PAGESIZE * device->multipage, layout->rb_profile_begin, layout->rb_profile_end, rb_profile_end); + rc = dc_rbstream_new (&rbstream, abstract, PAGESIZE, PAGESIZE * device->multipage, layout->rb_profile_begin, layout->rb_profile_end, rb_profile_end, DC_RBSTREAM_BACKWARD); if (rc != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); return rc; diff --git a/src/rbstream.c b/src/rbstream.c index 8e7e17a..f4da2d7 100644 --- a/src/rbstream.c +++ b/src/rbstream.c @@ -28,11 +28,13 @@ struct dc_rbstream_t { dc_device_t *device; + dc_rbstream_direction_t direction; unsigned int pagesize; unsigned int packetsize; unsigned int begin; unsigned int end; unsigned int address; + unsigned int offset; unsigned int available; unsigned int skip; unsigned char cache[]; @@ -53,7 +55,7 @@ iceil (unsigned int x, unsigned int n) } dc_status_t -dc_rbstream_new (dc_rbstream_t **out, dc_device_t *device, unsigned int pagesize, unsigned int packetsize, unsigned int begin, unsigned int end, unsigned int address) +dc_rbstream_new (dc_rbstream_t **out, dc_device_t *device, unsigned int pagesize, unsigned int packetsize, unsigned int begin, unsigned int end, unsigned int address, dc_rbstream_direction_t direction) { dc_rbstream_t *rbstream = NULL; @@ -104,27 +106,31 @@ dc_rbstream_new (dc_rbstream_t **out, dc_device_t *device, unsigned int pagesize } rbstream->device = device; + rbstream->direction = direction; rbstream->pagesize = pagesize; rbstream->packetsize = packetsize; rbstream->begin = begin; rbstream->end = end; - rbstream->address = iceil(address, pagesize); + if (direction == DC_RBSTREAM_FORWARD) { + rbstream->address = ifloor(address, pagesize); + rbstream->skip = address - rbstream->address; + } else { + rbstream->address = iceil(address, pagesize); + rbstream->skip = rbstream->address - address; + } + rbstream->offset = 0; rbstream->available = 0; - rbstream->skip = rbstream->address - address; *out = rbstream; return DC_STATUS_SUCCESS; } -dc_status_t -dc_rbstream_read (dc_rbstream_t *rbstream, dc_event_progress_t *progress, unsigned char data[], unsigned int size) +static dc_status_t +dc_rbstream_read_backward (dc_rbstream_t *rbstream, dc_event_progress_t *progress, unsigned char data[], unsigned int size) { dc_status_t rc = DC_STATUS_SUCCESS; - if (rbstream == NULL) - return DC_STATUS_INVALIDARGS; - unsigned int nbytes = 0; unsigned int offset = size; while (nbytes < size) { @@ -171,6 +177,73 @@ dc_rbstream_read (dc_rbstream_t *rbstream, dc_event_progress_t *progress, unsign return rc; } +static dc_status_t +dc_rbstream_read_forward (dc_rbstream_t *rbstream, dc_event_progress_t *progress, unsigned char data[], unsigned int size) +{ + dc_status_t rc = DC_STATUS_SUCCESS; + + unsigned int nbytes = 0; + while (nbytes < size) { + if (rbstream->available == 0) { + // Handle the ringbuffer wrap point. + if (rbstream->address == rbstream->end) + rbstream->address = rbstream->begin; + + // Calculate the packet size. + unsigned int len = rbstream->packetsize; + if (rbstream->address + len > rbstream->end) + len = rbstream->end - rbstream->address; + + // Calculate the excess number of bytes. + unsigned int extra = rbstream->packetsize - len; + + // Read the packet into the cache. + rc = dc_device_read (rbstream->device, rbstream->address - extra, rbstream->cache, rbstream->packetsize); + if (rc != DC_STATUS_SUCCESS) + return rc; + + // Move to the begin of the next packet. + rbstream->address += len; + + rbstream->offset = extra + rbstream->skip; + rbstream->available = len - rbstream->skip; + rbstream->skip = 0; + } + + unsigned int length = rbstream->available; + if (nbytes + length > size) + length = size - nbytes; + + memcpy (data + nbytes, rbstream->cache + rbstream->offset, length); + + rbstream->offset += length; + rbstream->available -= length; + + // Update and emit a progress event. + if (progress) { + progress->current += length; + device_event_emit (rbstream->device, DC_EVENT_PROGRESS, progress); + } + + nbytes += length; + } + + return rc; +} + +dc_status_t +dc_rbstream_read (dc_rbstream_t *rbstream, dc_event_progress_t *progress, unsigned char data[], unsigned int size) +{ + if (rbstream == NULL) + return DC_STATUS_INVALIDARGS; + + if (rbstream->direction == DC_RBSTREAM_FORWARD) { + return dc_rbstream_read_forward (rbstream, progress, data, size); + } else { + return dc_rbstream_read_backward (rbstream, progress, data, size); + } +} + dc_status_t dc_rbstream_free (dc_rbstream_t *rbstream) { diff --git a/src/rbstream.h b/src/rbstream.h index be1d8f6..2ea0767 100644 --- a/src/rbstream.h +++ b/src/rbstream.h @@ -33,6 +33,14 @@ extern "C" { */ typedef struct dc_rbstream_t dc_rbstream_t; +/** + * The ringbuffer read direction. + */ +typedef enum dc_rbstream_direction_t { + DC_RBSTREAM_FORWARD, + DC_RBSTREAM_BACKWARD +} dc_rbstream_direction_t; + /** * Create a new ringbuffer stream. * @@ -43,11 +51,12 @@ typedef struct dc_rbstream_t dc_rbstream_t; * @param[in] begin The ringbuffer begin address. * @param[in] end The ringbuffer end address. * @param[in] address The stream start address. + * @param[in] direction The ringbuffer read direction. * @returns #DC_STATUS_SUCCESS on success, or another #dc_status_t code * on failure. */ dc_status_t -dc_rbstream_new (dc_rbstream_t **rbstream, dc_device_t *device, unsigned int pagesize, unsigned int packetsize, unsigned int begin, unsigned int end, unsigned int address); +dc_rbstream_new (dc_rbstream_t **rbstream, dc_device_t *device, unsigned int pagesize, unsigned int packetsize, unsigned int begin, unsigned int end, unsigned int address, dc_rbstream_direction_t direction); /** * Read data from the ringbuffer stream. diff --git a/src/seac_screen.c b/src/seac_screen.c index 9b3796f..5c566aa 100644 --- a/src/seac_screen.c +++ b/src/seac_screen.c @@ -546,7 +546,7 @@ seac_screen_device_foreach (dc_device_t *abstract, dc_dive_callback_t callback, // Create the ringbuffer stream. dc_rbstream_t *rbstream = NULL; - status = dc_rbstream_new (&rbstream, abstract, SZ_READ, SZ_READ, RB_PROFILE_BEGIN, RB_PROFILE_END, eop); + status = dc_rbstream_new (&rbstream, abstract, SZ_READ, SZ_READ, RB_PROFILE_BEGIN, RB_PROFILE_END, eop, DC_RBSTREAM_BACKWARD); if (status != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); goto error_free_profile; diff --git a/src/suunto_common2.c b/src/suunto_common2.c index 063ea2b..d33f725 100644 --- a/src/suunto_common2.c +++ b/src/suunto_common2.c @@ -307,7 +307,7 @@ suunto_common2_device_foreach (dc_device_t *abstract, dc_dive_callback_t callbac // Create the ringbuffer stream. dc_rbstream_t *rbstream = NULL; - rc = dc_rbstream_new (&rbstream, abstract, 1, SZ_PACKET, layout->rb_profile_begin, layout->rb_profile_end, end); + rc = dc_rbstream_new (&rbstream, abstract, 1, SZ_PACKET, layout->rb_profile_begin, layout->rb_profile_end, end, DC_RBSTREAM_BACKWARD); if (rc != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); return rc; diff --git a/src/zeagle_n2ition3.c b/src/zeagle_n2ition3.c index 420ac0c..53ac363 100644 --- a/src/zeagle_n2ition3.c +++ b/src/zeagle_n2ition3.c @@ -326,7 +326,7 @@ zeagle_n2ition3_device_foreach (dc_device_t *abstract, dc_dive_callback_t callba // Create the ringbuffer stream. dc_rbstream_t *rbstream = NULL; - rc = dc_rbstream_new (&rbstream, abstract, 1, SZ_PACKET, RB_PROFILE_BEGIN, RB_PROFILE_END, eop); + rc = dc_rbstream_new (&rbstream, abstract, 1, SZ_PACKET, RB_PROFILE_BEGIN, RB_PROFILE_END, eop, DC_RBSTREAM_BACKWARD); if (rc != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to create the ringbuffer stream."); return rc;