From cfc1a68b0d01b4e61a2c828a2a8149ee3c5cbfb1 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Sun, 12 Jul 2015 21:19:28 +0200 Subject: [PATCH 1/3] Handle invalid logbook pointers as a fatal error. Until now, an invalid logbook pointer was silently ignored and handled as an empty ringbuffer. But this hides real errors, which is worse than failing if no dives are present. Trying to download dives from an empty device should be a rather uncommon scenario anyway. --- src/oceanic_common.c | 48 +++++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/src/oceanic_common.c b/src/oceanic_common.c index f29d176..e8dbba4 100644 --- a/src/oceanic_common.c +++ b/src/oceanic_common.c @@ -246,39 +246,33 @@ oceanic_common_device_foreach (dc_device_t *abstract, dc_dive_callback_t callbac // Get the logbook pointers. unsigned int rb_logbook_first = array_uint16_le (pointers + 4); unsigned int rb_logbook_last = array_uint16_le (pointers + 6); - - // Convert the first/last pointers to begin/end/count pointers. - unsigned int rb_logbook_entry_begin, rb_logbook_entry_end, - rb_logbook_entry_size; if (rb_logbook_first < layout->rb_logbook_begin || rb_logbook_first >= layout->rb_logbook_end || rb_logbook_last < layout->rb_logbook_begin || rb_logbook_last >= layout->rb_logbook_end) { - // One of the pointers is outside the valid ringbuffer area. - // Because some devices use invalid pointers to indicate an - // empty ringbuffer, we silently ignore the error and always - // consider the ringbuffer empty. - rb_logbook_entry_begin = layout->rb_logbook_begin; - rb_logbook_entry_end = layout->rb_logbook_begin; - rb_logbook_entry_size = 0; + ERROR (abstract->context, "Invalid logbook pointer detected."); + return DC_STATUS_DATAFORMAT; + } + + // Convert the first/last pointers to begin/end/count pointers. + unsigned int rb_logbook_entry_begin, rb_logbook_entry_end, + rb_logbook_entry_size; + if (layout->pt_mode_global == 0) { + rb_logbook_entry_begin = rb_logbook_first; + rb_logbook_entry_end = RB_LOGBOOK_INCR (rb_logbook_last, layout->rb_logbook_entry_size, layout); + rb_logbook_entry_size = RB_LOGBOOK_DISTANCE (rb_logbook_first, rb_logbook_last, layout) + layout->rb_logbook_entry_size; } else { - if (layout->pt_mode_global == 0) { - rb_logbook_entry_begin = rb_logbook_first; - rb_logbook_entry_end = RB_LOGBOOK_INCR (rb_logbook_last, layout->rb_logbook_entry_size, layout); - rb_logbook_entry_size = RB_LOGBOOK_DISTANCE (rb_logbook_first, rb_logbook_last, layout) + layout->rb_logbook_entry_size; - } else { - rb_logbook_entry_begin = rb_logbook_first; - rb_logbook_entry_end = rb_logbook_last; - rb_logbook_entry_size = RB_LOGBOOK_DISTANCE (rb_logbook_first, rb_logbook_last, layout); - // In a typical ringbuffer implementation with only two begin/end - // pointers, there is no distinction possible between an empty and - // a full ringbuffer. We always consider the ringbuffer full in - // that case, because an empty ringbuffer can be detected by - // inspecting the logbook entries once they are downloaded. - if (rb_logbook_first == rb_logbook_last) - rb_logbook_entry_size = layout->rb_logbook_end - layout->rb_logbook_begin; - } + rb_logbook_entry_begin = rb_logbook_first; + rb_logbook_entry_end = rb_logbook_last; + rb_logbook_entry_size = RB_LOGBOOK_DISTANCE (rb_logbook_first, rb_logbook_last, layout); + // In a typical ringbuffer implementation with only two begin/end + // pointers, there is no distinction possible between an empty and + // a full ringbuffer. We always consider the ringbuffer full in + // that case, because an empty ringbuffer can be detected by + // inspecting the logbook entries once they are downloaded. + if (rb_logbook_first == rb_logbook_last) + rb_logbook_entry_size = layout->rb_logbook_end - layout->rb_logbook_begin; } // Check whether the ringbuffer is full. From 76bd9783d4e5ae6030bd7685efc9ac36f394f8e7 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Sun, 12 Jul 2015 22:04:44 +0200 Subject: [PATCH 2/3] Add an exception for devices without a logbook ringbuffer. For devices without a logbook ringbuffer, such as the Oceanic Veo 1.0 and the Aeris XR-1 NX, the ringbuffer begin and end are identical. In this case, the changes in the previous commit will always result in a fatal error due to an invalid ringbuffer pointer. To avoid the error, we exit before trying to use the pointers. --- src/oceanic_common.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/oceanic_common.c b/src/oceanic_common.c index e8dbba4..2dde296 100644 --- a/src/oceanic_common.c +++ b/src/oceanic_common.c @@ -205,6 +205,9 @@ oceanic_common_device_foreach (dc_device_t *abstract, dc_dive_callback_t callbac progress.maximum = 2 * PAGESIZE + (layout->rb_profile_end - layout->rb_profile_begin) + (layout->rb_logbook_end - layout->rb_logbook_begin); + if (layout->rb_logbook_begin == layout->rb_logbook_end) { + progress.maximum -= PAGESIZE; + } device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); // Emit a vendor event. @@ -235,6 +238,13 @@ oceanic_common_device_foreach (dc_device_t *abstract, dc_dive_callback_t callbac devinfo.serial = id[11] * 10000 + id[12] * 100 + id[13]; device_event_emit (abstract, DC_EVENT_DEVINFO, &devinfo); + // For devices without a logbook ringbuffer, downloading dives isn't + // possible. This is not considered a fatal error, but handled as if there + // are no dives present. + if (layout->rb_logbook_begin == layout->rb_logbook_end) { + return DC_STATUS_SUCCESS; + } + // Read the pointer data. unsigned char pointers[PAGESIZE] = {0}; rc = dc_device_read (abstract, layout->cf_pointers, pointers, sizeof (pointers)); From 4795e1b6ce8a957bde70f5ca41d7e347f2c24743 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Sun, 12 Jul 2015 22:13:22 +0200 Subject: [PATCH 3/3] Prevent a zero length memory allocation. In theory this shouldn't be possible, but it doesn't hurt to check the length explicitely. --- src/oceanic_common.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/oceanic_common.c b/src/oceanic_common.c index 2dde296..c51836f 100644 --- a/src/oceanic_common.c +++ b/src/oceanic_common.c @@ -315,6 +315,11 @@ oceanic_common_device_foreach (dc_device_t *abstract, dc_dive_callback_t callbac rb_logbook_page_size; device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + // Exit if there are no dives. + if (rb_logbook_page_size == 0) { + return DC_STATUS_SUCCESS; + } + // Memory buffer for the logbook entries. unsigned char *logbooks = (unsigned char *) malloc (rb_logbook_page_size); if (logbooks == NULL)