V2: Coverity / static analysis based cleanups

List overview All Threads
Download

newer

older

V3: Coverity / static analysis...

Any interest in a VR3 integration?

Dirk Hohndel

3 Jan 2018 3 Jan '18

7:35 p.m.

This is the second version of a series of 14 patches against the current tip of the Subsurface-branch of libdivecomputer. I believe that all of them should apply to upstream master (but haven't verified that).

I have incorporated Jef's feedback on the first version. One change compared to my email responses to that feedback is that I ended up not modifying patch 12/14 (consistently check return value of iostream) as it made the code really ugly and as Jef said, not resetting the timeout doesn't really matter.

Linus, based on your feedback and Jef's comment I rewrote 7/14; can you please double check that I didn't mess this up by combining it all in one operation?

Happy to get more feedback / comments.

Thanks

[PATCH 01/14] Cleanup: correctly handle upper bound of array [PATCH 02/14] Cleanup: avoid memory leaks [PATCH 03/14] Cleanup: avoid memory leak [PATCH 04/14] Cleanup: avoid memory leak [PATCH 05/14] Cleanup: avoid memory leak [PATCH 06/14] Cleanup: ensure string is 0 terminated [PATCH 07/14] Cleanup: avoid undefined shift operation [PATCH 08/14] Cleanup: remove dead code and return the correct return [PATCH 09/14] Cleanup: dc_buffer_clear() should be a void function [PATCH 10/14] Cleanup: check return value of ioctl() [PATCH 11/14] Cleanup: check error return values of buffer handling [PATCH 12/14] Cleanup: consistently check return value of iostream [PATCH 13/14] Cleanup: bail on error [PATCH 14/14] Cleanup: remove confusing NULL check

Show replies by date

Dirk Hohndel

3 Jan 3 Jan

7:35 p.m.

New subject: [PATCH 01/14] Cleanup: correctly handle upper bound of array

Coverity CID 207684 Coverity CID 207724 Coverity CID 207728

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/suunto_eonsteel_parser.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/suunto_eonsteel_parser.c b/src/suunto_eonsteel_parser.c index 429e8626a9e4..7a64e5026c9c 100644 --- a/src/suunto_eonsteel_parser.c +++ b/src/suunto_eonsteel_parser.c @@ -217,7 +217,7 @@ static int fill_in_group_details(suunto_eonsteel_parser_t *eon, struct type_desc long index;

index = strtol(grp, &end, 10); - if (index < 0 || index > MAXTYPE || end == grp) { + if (index < 0 || index >= MAXTYPE || end == grp) { ERROR(eon->base.context, "Group type descriptor '%s' does not parse", desc->desc); break; } @@ -344,7 +344,7 @@ static int record_type(suunto_eonsteel_parser_t *eon, unsigned short type, const } } while ((name = next) != NULL);

- if (type > MAXTYPE) { + if (type >= MAXTYPE) { ERROR(eon->base.context, "Type out of range (%04x: '%s' '%s' '%s')", type, desc.desc ? desc.desc : "", @@ -413,7 +413,7 @@ static int traverse_entry(suunto_eonsteel_parser_t *eon, const unsigned char *p, end += 4; }

- if (type > MAXTYPE || !eon->type_desc[type].desc) { + if (type >= MAXTYPE || !eon->type_desc[type].desc) { HEXDUMP(eon->base.context, DC_LOGLEVEL_DEBUG, "last", last, 16); HEXDUMP(eon->base.context, DC_LOGLEVEL_DEBUG, "this", begin, 16); } else {

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 02/14] Cleanup: avoid memory leaks

Coverity CID 207730 Coverity CID 207747

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/suunto_eonsteel_parser.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/src/suunto_eonsteel_parser.c b/src/suunto_eonsteel_parser.c index 7a64e5026c9c..ffdd172d367f 100644 --- a/src/suunto_eonsteel_parser.c +++ b/src/suunto_eonsteel_parser.c @@ -776,10 +776,12 @@ static void sample_setpoint_type(const struct type_desc *desc, struct sample_dat sample.ppo2 = info->eon->cache.customsetpoint; else { DEBUG(info->eon->base.context, "sample_setpoint_type(%u) unknown type '%s'", value, type); + free((void *)type); return; }

if (info->callback) info->callback(DC_SAMPLE_SETPOINT, sample, info->userdata); + free((void *)type); }

// uint32 @@ -1125,6 +1127,7 @@ static int add_gas_type(suunto_eonsteel_parser_t *eon, const struct type_desc *d

eon->cache.initialized |= 1 << DC_FIELD_GASMIX_COUNT; eon->cache.initialized |= 1 << DC_FIELD_TANK_COUNT; + free((void *)name); return 0; }

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 03/14] Cleanup: avoid memory leak

Coverity CID 207807

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/scubapro_g2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/scubapro_g2.c b/src/scubapro_g2.c index dcef6278593c..c6c533fbe05e 100644 --- a/src/scubapro_g2.c +++ b/src/scubapro_g2.c @@ -245,7 +245,8 @@ scubapro_g2_device_open(dc_device_t **out, dc_context_t *context, const char *na const struct usb_id *id = get_usb_id(model); if (!id) { ERROR(context, "Unknown USB ID for Scubapro model %#04x", model); - return DC_STATUS_IO; + status = DC_STATUS_IO; + goto error_free; } status = dc_usbhid_custom_io(context, id->vendor, id->device); }

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 04/14] Cleanup: avoid memory leak

Coverity CID 207773

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/reefnet_sensusultra.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/src/reefnet_sensusultra.c b/src/reefnet_sensusultra.c index fd3d39961f46..a6aef5aca4db 100644 --- a/src/reefnet_sensusultra.c +++ b/src/reefnet_sensusultra.c @@ -716,6 +716,7 @@ reefnet_sensusultra_device_foreach (dc_device_t *abstract, dc_dive_callback_t ca

// Prepend the packet to the buffer. if (!dc_buffer_prepend (buffer, packet + 2, SZ_PACKET)) { + dc_buffer_free (buffer); ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; }

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 05/14] Cleanup: avoid memory leak

Coverity CID 207731

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/divesystem_idive.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/divesystem_idive.c b/src/divesystem_idive.c index 46ecb3421620..642aaee4a05e 100644 --- a/src/divesystem_idive.c +++ b/src/divesystem_idive.c @@ -500,8 +500,10 @@ divesystem_idive_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb (idx ) & 0xFF, (idx >> 8) & 0xFF}; rc = divesystem_idive_transfer (device, cmd_sample, sizeof(cmd_sample), packet, commands->sample.size * commands->nsamples, &errcode); - if (rc != DC_STATUS_SUCCESS) + if (rc != DC_STATUS_SUCCESS) { + dc_buffer_free(buffer); return rc; + }

// If the number of samples is not an exact multiple of the // number of samples per packet, then the last packet

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 06/14] Cleanup: ensure string is 0 terminated

The Linux kernel uses the sir_name as a standard C string (in one instance copying it into a 60 char buffer using kstrncpy with a length limit of 60), we therefore need to ensure that it is 0 terminated.

Since the existing code didn't notify the caller if we were truncating the string at 25 characters, I didn't add such a warning/error for truncating at 24 characters.

I was not able to find documentation on how Windows uses irdaServiceName so I didn't change that code.

In both cases I replaced the hardcoded length of 25 with a sizeof() argument (but both Linux and Windows hard code that length in their headers, so it seems unlikely this would ever change).

Coverity CID 207790

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/irda.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/irda.c b/src/irda.c index 149808aaa5c2..0ad227d70f40 100644 --- a/src/irda.c +++ b/src/irda.c @@ -222,17 +222,19 @@ dc_irda_connect_name (dc_iostream_t *abstract, unsigned int address, const char peer.irdaDeviceID[2] = (address >> 16) & 0xFF; peer.irdaDeviceID[3] = (address >> 24) & 0xFF; if (name) - strncpy (peer.irdaServiceName, name, 25); + strncpy (peer.irdaServiceName, name, sizeof(peer.irdaServiceName)); else - memset (peer.irdaServiceName, 0x00, 25); + memset (peer.irdaServiceName, 0x00, sizeof(peer.irdaServiceName)); #else struct sockaddr_irda peer; peer.sir_family = AF_IRDA; peer.sir_addr = address; - if (name) - strncpy (peer.sir_name, name, 25); - else - memset (peer.sir_name, 0x00, 25); + if (name) { + strncpy (peer.sir_name, name, sizeof(peer.sir_name) - 1); + peer.sir_name[sizeof(peer.sir_name) - 1] = '\0'; + } else { + memset (peer.sir_name, 0x00, sizeof(peer.sir_name)); + } #endif

return dc_socket_connect (&device->base, (struct sockaddr *) &peer, sizeof (peer));

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 07/14] Cleanup: avoid undefined shift operation

Shifting a 32bit value by 32 is undefined. Instead of using shifts to create the mask, explicitly create it by subtracting 1 from the signbit value (and using bitwise NOT to fill all the higher bits).

Coverity CID 207769

Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/uwatec_smart_parser.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/uwatec_smart_parser.c b/src/uwatec_smart_parser.c index a70acb7b91c6..053cad9f2914 100644 --- a/src/uwatec_smart_parser.c +++ b/src/uwatec_smart_parser.c @@ -886,15 +886,14 @@ uwatec_smart_fixsignbit (unsigned int x, unsigned int n) return 0;

unsigned int signbit = (1 << (n - 1)); - unsigned int mask = (0xFFFFFFFF << n);

// When turning a two's-complement number with a certain number // of bits into one with more bits, the sign bit must be repeated // in all the extra bits. if ((x & signbit) == signbit) - return x | mask; + return x | ~(signbit - 1); else - return x & ~mask; + return x & (signbit - 1); }

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 08/14] Cleanup: remove dead code and return the correct return code

Coverity CID 207700

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/uwatec_smart.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/src/uwatec_smart.c b/src/uwatec_smart.c index bb67131134a9..d30bee6f3f0f 100644 --- a/src/uwatec_smart.c +++ b/src/uwatec_smart.c @@ -213,17 +213,10 @@ error_free: static dc_status_t uwatec_smart_device_close (dc_device_t *abstract) { - dc_status_t status = DC_STATUS_SUCCESS; uwatec_smart_device_t *device = (uwatec_smart_device_t*) abstract; - dc_status_t rc = DC_STATUS_SUCCESS; - - // Close the device. - rc = dc_iostream_close (device->iostream); - if (status != DC_STATUS_SUCCESS) { - dc_status_set_error(&status, rc); - }

- return status; + // Close the device and pass up the return code. + return dc_iostream_close (device->iostream); }

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 09/14] Cleanup: dc_buffer_clear() should be a void function

We sometimes ignored its return value, sometimes we didn't. Semantically, clearing a non-existing buffer is just a noop.

Careful - one side effect of the old semantic was that dc_buffer_clear() plus a bail on failure could be used to test if the buffer had been allocated. Right now there is at least one call site where we later call dc_buffer_apend() without checking its return value. This will be fixed in a later commit in this series.

Coverity CID 207756 Coverity CID 207714 Coverity CID 207800 Coverity CID 207729

diff --git a/include/libdivecomputer/buffer.h b/include/libdivecomputer/buffer.h index 2af4822ad8b5..a807187d48d1 100644 --- a/include/libdivecomputer/buffer.h +++ b/include/libdivecomputer/buffer.h @@ -36,7 +36,7 @@ dc_buffer_new (size_t capacity); void dc_buffer_free (dc_buffer_t *buffer);

-int +void dc_buffer_clear (dc_buffer_t *buffer);

int diff --git a/src/atomics_cobalt.c b/src/atomics_cobalt.c index 7c7dccb04876..a5ce98c9a302 100644 --- a/src/atomics_cobalt.c +++ b/src/atomics_cobalt.c @@ -258,10 +258,7 @@ atomics_cobalt_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, return DC_STATUS_CANCELLED;

// Erase the current contents of the buffer. - if (!dc_buffer_clear (buffer)) { - ERROR (abstract->context, "Insufficient buffer space available."); - return DC_STATUS_NOMEMORY; - } + dc_buffer_clear (buffer);

// Send the command to the dive computer. uint8_t bRequest = 0; diff --git a/src/buffer.c b/src/buffer.c index 5b203fb261d9..10d84ab67fb6 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -67,19 +67,17 @@ dc_buffer_free (dc_buffer_t *buffer) }

-int +void dc_buffer_clear (dc_buffer_t *buffer) { - if (buffer == NULL) - return 0; - - buffer->offset = 0; - buffer->size = 0; - - return 1; + if (buffer != NULL) { + buffer->offset = 0; + buffer->size = 0; + } }

+// static, internal function; only called with buffer != NULL static size_t dc_buffer_expand_calc (dc_buffer_t *buffer, size_t n) { @@ -92,6 +90,7 @@ dc_buffer_expand_calc (dc_buffer_t *buffer, size_t n) }

+// static, internal function; only called with buffer != NULL static int dc_buffer_expand_append (dc_buffer_t *buffer, size_t n) { @@ -123,6 +122,7 @@ dc_buffer_expand_append (dc_buffer_t *buffer, size_t n) }

+// static, internal function; only called with buffer != NULL static int dc_buffer_expand_prepend (dc_buffer_t *buffer, size_t n) { diff --git a/src/citizen_aqualand.c b/src/citizen_aqualand.c index 3af5d528e863..cfcdbb85ee81 100644 --- a/src/citizen_aqualand.c +++ b/src/citizen_aqualand.c @@ -152,12 +152,8 @@ citizen_aqualand_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) dc_status_t status = DC_STATUS_SUCCESS; citizen_aqualand_device_t *device = (citizen_aqualand_device_t *) abstract;

- // Erase the current contents of the buffer and - // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer)) { - ERROR (abstract->context, "Insufficient buffer space available."); - return DC_STATUS_NOMEMORY; - } + // Erase the current contents of the buffer + dc_buffer_clear (buffer);

dc_iostream_set_dtr (device->iostream, 1);

diff --git a/src/cochran_commander.c b/src/cochran_commander.c index e21ec7e3f0bc..1225c3808ac8 100644 --- a/src/cochran_commander.c +++ b/src/cochran_commander.c @@ -842,11 +842,8 @@ cochran_commander_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) unsigned int config_size = sizeof(config); unsigned int size = device->layout->rb_profile_end - device->layout->rb_logbook_begin;

- // Make sure buffer is good. - if (!dc_buffer_clear(buffer)) { - ERROR (abstract->context, "Uninitialized buffer."); - return DC_STATUS_INVALIDARGS; - } + // Erase the buffer content + dc_buffer_clear(buffer);

// Reserve space if (!dc_buffer_resize(buffer, size)) { diff --git a/src/cressi_edy.c b/src/cressi_edy.c index 80e4fac4d854..e5302ace41e4 100644 --- a/src/cressi_edy.c +++ b/src/cressi_edy.c @@ -391,7 +391,8 @@ cressi_edy_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, device->layout->memsize)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, device->layout->memsize)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/cressi_leonardo.c b/src/cressi_leonardo.c index 402128821fdd..9ea323b6dddc 100644 --- a/src/cressi_leonardo.c +++ b/src/cressi_leonardo.c @@ -322,7 +322,8 @@ cressi_leonardo_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/diverite_nitekq.c b/src/diverite_nitekq.c index 8524b568315f..9ed1fa7df538 100644 --- a/src/diverite_nitekq.c +++ b/src/diverite_nitekq.c @@ -259,7 +259,8 @@ diverite_nitekq_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) unsigned char packet[256] = {0};

// Erase the current contents of the buffer. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, SZ_PACKET + SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, SZ_PACKET + SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/hw_ostc.c b/src/hw_ostc.c index 36d608086e96..4e4335caf291 100644 --- a/src/hw_ostc.c +++ b/src/hw_ostc.c @@ -220,10 +220,7 @@ hw_ostc_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) hw_ostc_device_t *device = (hw_ostc_device_t*) abstract;

// Enable progress notifications. dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; @@ -507,10 +504,7 @@ hw_ostc_device_screenshot (dc_device_t *abstract, dc_buffer_t *buffer, hw_ostc_f return DC_STATUS_INVALIDARGS;

// Bytes per pixel (RGB formats only). unsigned int bpp = 0; diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index e240342cc2f6..eca8dfbc30e8 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -1556,10 +1556,7 @@ hw_ostc3_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;

// Enable progress notifications. dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; diff --git a/src/mares_darwin.c b/src/mares_darwin.c index 4f87f6fcd14a..308477cda6b6 100644 --- a/src/mares_darwin.c +++ b/src/mares_darwin.c @@ -221,7 +221,8 @@ mares_darwin_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, device->layout->memsize)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, device->layout->memsize)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index 990f46657bdb..b56da27a5b8a 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -399,7 +399,8 @@ mares_iconhd_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, device->layout->memsize)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, device->layout->memsize)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/mares_nemo.c b/src/mares_nemo.c index d6d4e015db63..b3d262a5c18c 100644 --- a/src/mares_nemo.c +++ b/src/mares_nemo.c @@ -195,7 +195,8 @@ mares_nemo_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, MEMORYSIZE)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, MEMORYSIZE)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/mares_puck.c b/src/mares_puck.c index 5006b43ef0b9..23f4d9e794c2 100644 --- a/src/mares_puck.c +++ b/src/mares_puck.c @@ -226,7 +226,8 @@ mares_puck_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, device->layout->memsize)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, device->layout->memsize)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/oceanic_common.c b/src/oceanic_common.c index 43bc06ddd0db..7f67418794b6 100644 --- a/src/oceanic_common.c +++ b/src/oceanic_common.c @@ -158,7 +158,8 @@ oceanic_common_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, device->layout->memsize)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, device->layout->memsize)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } @@ -188,8 +189,7 @@ oceanic_common_device_logbook (dc_device_t *abstract, dc_event_progress_t *progr const oceanic_common_layout_t *layout = device->layout;

// Erase the buffer. - if (!dc_buffer_clear (logbook)) - return DC_STATUS_NOMEMORY; + dc_buffer_clear (logbook);

// For devices without a logbook ringbuffer, downloading dives isn't // possible. This is not considered a fatal error, but handled as if there diff --git a/src/oceanic_vtpro.c b/src/oceanic_vtpro.c index e1ba31ced88a..ca3a2fc2a479 100644 --- a/src/oceanic_vtpro.c +++ b/src/oceanic_vtpro.c @@ -299,8 +299,7 @@ oceanic_aeris500ai_device_logbook (dc_device_t *abstract, dc_event_progress_t *p const oceanic_common_layout_t *layout = device->base.layout;

// Erase the buffer. - if (!dc_buffer_clear (logbook)) - return DC_STATUS_NOMEMORY; + dc_buffer_clear (logbook);

// Read the pointer data. unsigned char pointers[PAGESIZE] = {0}; diff --git a/src/reefnet_sensus.c b/src/reefnet_sensus.c index 0fc6e5228cc2..29b9bb1c8429 100644 --- a/src/reefnet_sensus.c +++ b/src/reefnet_sensus.c @@ -281,7 +281,8 @@ reefnet_sensus_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/reefnet_sensuspro.c b/src/reefnet_sensuspro.c index e50cb44937f1..b6bdd6bfe751 100644 --- a/src/reefnet_sensuspro.c +++ b/src/reefnet_sensuspro.c @@ -265,7 +265,8 @@ reefnet_sensuspro_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/reefnet_sensusultra.c b/src/reefnet_sensusultra.c index a6aef5aca4db..d2c73817efd7 100644 --- a/src/reefnet_sensusultra.c +++ b/src/reefnet_sensusultra.c @@ -381,7 +381,8 @@ reefnet_sensusultra_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/scubapro_g2.c b/src/scubapro_g2.c index c6c533fbe05e..977932d3933c 100644 --- a/src/scubapro_g2.c +++ b/src/scubapro_g2.c @@ -308,10 +308,7 @@ scubapro_g2_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) dc_status_t rc = DC_STATUS_SUCCESS;

// Enable progress notifications. dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; diff --git a/src/shearwater_common.c b/src/shearwater_common.c index bad14c8ba5d8..f5004552b68a 100644 --- a/src/shearwater_common.c +++ b/src/shearwater_common.c @@ -365,10 +365,7 @@ shearwater_common_download (shearwater_common_device_t *device, dc_buffer_t *buf unsigned char response[SZ_PACKET];

// Enable progress notifications. unsigned int initial = 0, current = 0, maximum = 3 + size + 1; @@ -480,10 +477,7 @@ shearwater_common_identifier (shearwater_common_device_t *device, dc_buffer_t *b dc_status_t rc = DC_STATUS_SUCCESS;

// Erase the buffer. - if (!dc_buffer_clear (buffer)) { - ERROR (abstract->context, "Insufficient buffer space available."); - return DC_STATUS_NOMEMORY; - } + dc_buffer_clear (buffer);

// Transfer the request. unsigned int n = 0; diff --git a/src/shearwater_predator.c b/src/shearwater_predator.c index a47d4141de9a..6d1214f4abb2 100644 --- a/src/shearwater_predator.c +++ b/src/shearwater_predator.c @@ -128,7 +128,8 @@ shearwater_predator_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) shearwater_common_device_t *device = (shearwater_common_device_t *) abstract;

// Erase the current contents of the buffer. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/suunto_common2.c b/src/suunto_common2.c index 4aa1699ef8f1..40d32d22e9b5 100644 --- a/src/suunto_common2.c +++ b/src/suunto_common2.c @@ -207,7 +207,8 @@ suunto_common2_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, device->layout->memsize)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, device->layout->memsize)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/suunto_eon.c b/src/suunto_eon.c index f41fae6e783c..1fbcaa11c74b 100644 --- a/src/suunto_eon.c +++ b/src/suunto_eon.c @@ -151,7 +151,8 @@ suunto_eon_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/suunto_solution.c b/src/suunto_solution.c index 2e9e14efa707..bbeaf8e90132 100644 --- a/src/suunto_solution.c +++ b/src/suunto_solution.c @@ -145,7 +145,8 @@ suunto_solution_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/suunto_vyper.c b/src/suunto_vyper.c index ba5ad20047b7..cc4680bd7dbe 100644 --- a/src/suunto_vyper.c +++ b/src/suunto_vyper.c @@ -333,10 +333,7 @@ suunto_vyper_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, dc return DC_STATUS_CANCELLED;

// Send the command to the dive computer. unsigned char command[3] = {init ? 0x08 : 0x09, 0xA5, 0x00}; @@ -448,7 +445,8 @@ suunto_vyper_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) { // Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/uwatec_aladin.c b/src/uwatec_aladin.c index dc697c29cc44..8fcd918bfcf8 100644 --- a/src/uwatec_aladin.c +++ b/src/uwatec_aladin.c @@ -180,7 +180,8 @@ uwatec_aladin_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)

// Erase the current contents of the buffer and // pre-allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_reserve (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_reserve (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; } diff --git a/src/uwatec_memomouse.c b/src/uwatec_memomouse.c index 90466a0ef931..e1bd94978a68 100644 --- a/src/uwatec_memomouse.c +++ b/src/uwatec_memomouse.c @@ -256,10 +256,7 @@ uwatec_memomouse_read_packet_inner (uwatec_memomouse_device_t *device, dc_buffer dc_device_t *abstract = (dc_device_t *) device;

unsigned int nbytes = 0; unsigned int total = PACKETSIZE; @@ -451,10 +448,7 @@ uwatec_memomouse_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) dc_status_t rc = DC_STATUS_SUCCESS;

// Give the interface some time to notice the DTR // line change from a previous transfer (if any). diff --git a/src/uwatec_meridian.c b/src/uwatec_meridian.c index 5844c29e4a98..28e807dbdef4 100644 --- a/src/uwatec_meridian.c +++ b/src/uwatec_meridian.c @@ -289,10 +289,7 @@ uwatec_meridian_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) dc_status_t rc = DC_STATUS_SUCCESS;

// Enable progress notifications. dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; diff --git a/src/uwatec_smart.c b/src/uwatec_smart.c index d30bee6f3f0f..80d5b0285a8d 100644 --- a/src/uwatec_smart.c +++ b/src/uwatec_smart.c @@ -244,10 +244,7 @@ uwatec_smart_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) dc_status_t rc = DC_STATUS_SUCCESS;

// Enable progress notifications. dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; diff --git a/src/zeagle_n2ition3.c b/src/zeagle_n2ition3.c index 01b9d5eccc34..1717ea4e9d6f 100644 --- a/src/zeagle_n2ition3.c +++ b/src/zeagle_n2ition3.c @@ -265,7 +265,8 @@ zeagle_n2ition3_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) { // Erase the current contents of the buffer and // allocate the required amount of memory. - if (!dc_buffer_clear (buffer) || !dc_buffer_resize (buffer, SZ_MEMORY)) { + dc_buffer_clear (buffer); + if (!dc_buffer_resize (buffer, SZ_MEMORY)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; }

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 10/14] Cleanup: check return value of ioctl()

It's checked for all the other invocations...

Coverity CID 207796

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/serial_posix.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/serial_posix.c b/src/serial_posix.c index 1698df8159b3..9a9373e32fb9 100644 --- a/src/serial_posix.c +++ b/src/serial_posix.c @@ -262,7 +262,11 @@ dc_serial_close (dc_iostream_t *abstract)

#ifndef ENABLE_PTY // Disable exclusive access mode. - ioctl (device->fd, TIOCNXCL, NULL); + if (ioctl (device->fd, TIOCNXCL, NULL)) { + int errcode = errno; + SYSERROR (abstract->context, errcode); + dc_status_set_error(&status, syserror (errcode)); + } #endif

// Close the device.

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 11/14] Cleanup: check error return values of buffer handling

This is a farily big change and in some cases these checks are redundant as we reserved the necessary space already. But from a consistency perspective it makes more sense to always check the return value.

Coverity CID 207798

diff --git a/src/atomics_cobalt.c b/src/atomics_cobalt.c index a5ce98c9a302..5335eba94e47 100644 --- a/src/atomics_cobalt.c +++ b/src/atomics_cobalt.c @@ -297,7 +297,11 @@ atomics_cobalt_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, }

// Append the packet to the output buffer. - dc_buffer_append (buffer, packet, length); + if (!dc_buffer_append (buffer, packet, length)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } + nbytes += length;

// If we received fewer bytes than requested, the transfer is finished. diff --git a/src/citizen_aqualand.c b/src/citizen_aqualand.c index cfcdbb85ee81..f639efdd9566 100644 --- a/src/citizen_aqualand.c +++ b/src/citizen_aqualand.c @@ -184,7 +184,10 @@ citizen_aqualand_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return status; }

- dc_buffer_append(buffer, answer, sizeof (answer)); + if (!dc_buffer_append(buffer, answer, sizeof (answer))) { + ERROR (abstract->context, "Failed to allocate memory."); + return DC_STATUS_NOMEMORY; + }

// Send the command. status = dc_iostream_write (device->iostream, command, sizeof (command), NULL); diff --git a/src/diverite_nitekq.c b/src/diverite_nitekq.c index 9ed1fa7df538..11628f7e4066 100644 --- a/src/diverite_nitekq.c +++ b/src/diverite_nitekq.c @@ -298,7 +298,10 @@ diverite_nitekq_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return rc; }

- dc_buffer_append (buffer, packet, sizeof (packet)); + if (!dc_buffer_append (buffer, packet, sizeof (packet))) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + }

// Update and emit a progress event. progress.current += SZ_PACKET; @@ -323,7 +326,10 @@ diverite_nitekq_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return rc; }

// Update and emit a progress event. progress.current += SZ_PACKET; diff --git a/src/divesystem_idive.c b/src/divesystem_idive.c index 642aaee4a05e..6fcdfabafc5c 100644 --- a/src/divesystem_idive.c +++ b/src/divesystem_idive.c @@ -491,8 +491,14 @@ divesystem_idive_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);

dc_buffer_clear(buffer); - dc_buffer_reserve(buffer, commands->header.size + commands->sample.size * nsamples); - dc_buffer_append(buffer, packet, commands->header.size); + if (!dc_buffer_reserve(buffer, commands->header.size + commands->sample.size * nsamples)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } + if (!dc_buffer_append(buffer, packet, commands->header.size)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + }

for (unsigned int j = 0; j < nsamples; j += commands->nsamples) { unsigned int idx = j + 1; @@ -517,7 +523,10 @@ divesystem_idive_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb progress.current = i * NSTEPS + STEP(j + n + 1, nsamples + 1); device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);

- dc_buffer_append(buffer, packet, commands->sample.size * n); + if (!dc_buffer_append(buffer, packet, commands->sample.size * n)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } }

unsigned char *data = dc_buffer_get_data(buffer); diff --git a/src/hw_ostc.c b/src/hw_ostc.c index 4e4335caf291..ed81fdf293df 100644 --- a/src/hw_ostc.c +++ b/src/hw_ostc.c @@ -587,7 +587,10 @@ hw_ostc_device_screenshot (dc_device_t *abstract, dc_buffer_t *buffer, hw_ostc_f

if (format == HW_OSTC_FORMAT_RAW) { // Append the raw data to the output buffer. - dc_buffer_append (buffer, raw, nbytes); + if (!dc_buffer_append (buffer, raw, nbytes)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } } else { // Store the decompressed data in the output buffer. for (unsigned int i = 0; i < count; ++i) { diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index eca8dfbc30e8..15862c255443 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -1150,7 +1150,10 @@ hw_ostc3_firmware_readfile4 (dc_buffer_t *buffer, dc_context_t *context, const c size_t n = 0; unsigned char block[1024] = {0}; while ((n = fread (block, 1, sizeof (block), fp)) > 0) { - dc_buffer_append (buffer, block, n); + if (dc_buffer_append (buffer, block, n)) { + ERROR (context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } }

// Close the file. diff --git a/src/mares_nemo.c b/src/mares_nemo.c index b3d262a5c18c..fc8a9a389d36 100644 --- a/src/mares_nemo.c +++ b/src/mares_nemo.c @@ -256,15 +256,24 @@ mares_nemo_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) ERROR (abstract->context, "Both packets are not equal."); return DC_STATUS_PROTOCOL; } - dc_buffer_append (buffer, packet, PACKETSIZE); + if (!dc_buffer_append (buffer, packet, PACKETSIZE)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } } else if (crc1 == ccrc1) { // Only the first packet has a correct checksum. WARNING (abstract->context, "Only the first packet has a correct checksum."); - dc_buffer_append (buffer, packet, PACKETSIZE); + if (!dc_buffer_append (buffer, packet, PACKETSIZE)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } } else if (crc2 == ccrc2) { // Only the second packet has a correct checksum. WARNING (abstract->context, "Only the second packet has a correct checksum."); - dc_buffer_append (buffer, packet + PACKETSIZE + 1, PACKETSIZE); + if (!dc_buffer_append (buffer, packet + PACKETSIZE + 1, PACKETSIZE)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } } else { ERROR (abstract->context, "Unexpected answer checksum."); return DC_STATUS_PROTOCOL; diff --git a/src/oceanic_vtpro.c b/src/oceanic_vtpro.c index ca3a2fc2a479..cdb8943930b7 100644 --- a/src/oceanic_vtpro.c +++ b/src/oceanic_vtpro.c @@ -364,7 +364,10 @@ oceanic_aeris500ai_device_logbook (dc_device_t *abstract, dc_event_progress_t *p if (memcmp (answer, device->base.fingerprint, PAGESIZE / 2) == 0) { dc_buffer_clear (logbook); } else { - dc_buffer_append (logbook, answer, PAGESIZE / 2); + if (!dc_buffer_append (logbook, answer, PAGESIZE / 2)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + } } }

diff --git a/src/reefnet_sensus.c b/src/reefnet_sensus.c index 29b9bb1c8429..b6462309cb0d 100644 --- a/src/reefnet_sensus.c +++ b/src/reefnet_sensus.c @@ -344,7 +344,10 @@ reefnet_sensus_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return DC_STATUS_PROTOCOL; }

- dc_buffer_append (buffer, answer + 4, SZ_MEMORY); + if (!dc_buffer_append (buffer, answer + 4, SZ_MEMORY)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + }

return DC_STATUS_SUCCESS; } diff --git a/src/reefnet_sensuspro.c b/src/reefnet_sensuspro.c index b6bdd6bfe751..88c165028f12 100644 --- a/src/reefnet_sensuspro.c +++ b/src/reefnet_sensuspro.c @@ -308,7 +308,10 @@ reefnet_sensuspro_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return DC_STATUS_PROTOCOL; }

- dc_buffer_append (buffer, answer, SZ_MEMORY); + if (!dc_buffer_append (buffer, answer, SZ_MEMORY)) { + ERROR (abstract->context, "Insuffiecient buffer space."); + return DC_STATUS_NOMEMORY; + }

return DC_STATUS_SUCCESS; } diff --git a/src/suunto_eon.c b/src/suunto_eon.c index 1fbcaa11c74b..de5685558b53 100644 --- a/src/suunto_eon.c +++ b/src/suunto_eon.c @@ -209,7 +209,10 @@ suunto_eon_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return DC_STATUS_PROTOCOL; }

- dc_buffer_append (buffer, answer, SZ_MEMORY); + if (!dc_buffer_append (buffer, answer, SZ_MEMORY)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + }

return DC_STATUS_SUCCESS; } diff --git a/src/suunto_eonsteel.c b/src/suunto_eonsteel.c index cbf1b756d945..948d1d329076 100644 --- a/src/suunto_eonsteel.c +++ b/src/suunto_eonsteel.c @@ -593,7 +593,10 @@ static int read_file(suunto_eonsteel_device_t *eon, const char *filename, dc_buf

if (got > size) got = size; - dc_buffer_append(buf, result+8, got); + if (!dc_buffer_append(buf, result+8, got)) { + ERROR(eon->base.context, "Insufficient buffer space available."); + return -1; + } offset += got; size -= got; } @@ -863,7 +866,10 @@ suunto_eonsteel_device_foreach(dc_device_t *abstract, dc_dive_callback_t callbac // Reset the membuffer, put the 4-byte length at the head. dc_buffer_clear(file); put_le32(time, buf); - dc_buffer_append(file, buf, 4); + if (!dc_buffer_append(file, buf, 4)) { + ERROR(abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + }

// Then read the filename into the rest of the buffer rc = read_file(eon, pathname, file); diff --git a/src/suunto_vyper.c b/src/suunto_vyper.c index cc4680bd7dbe..586a01716dfd 100644 --- a/src/suunto_vyper.c +++ b/src/suunto_vyper.c @@ -410,7 +410,10 @@ suunto_vyper_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, dc // transfer is finished. This approach leaves no data behind in // the serial receive buffer, and if this packet is part of the // last incomplete dive, no error has to be reported at all. - dc_buffer_append (buffer, answer + 2, len); + if (!dc_buffer_append (buffer, answer + 2, len)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + }

nbytes += len;

diff --git a/src/uwatec_aladin.c b/src/uwatec_aladin.c index 8fcd918bfcf8..d6993db4ad8d 100644 --- a/src/uwatec_aladin.c +++ b/src/uwatec_aladin.c @@ -250,7 +250,10 @@ uwatec_aladin_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) clock.devtime = device->devtime; device_event_emit (abstract, DC_EVENT_CLOCK, &clock);

return DC_STATUS_SUCCESS; } diff --git a/src/uwatec_memomouse.c b/src/uwatec_memomouse.c index e1bd94978a68..e55b13dffb56 100644 --- a/src/uwatec_memomouse.c +++ b/src/uwatec_memomouse.c @@ -306,7 +306,10 @@ uwatec_memomouse_read_packet_inner (uwatec_memomouse_device_t *device, dc_buffer }

// Append the packet to the buffer. - dc_buffer_append (buffer, packet + 1, length); + if (!dc_buffer_append (buffer, packet + 1, length)) { + ERROR (abstract->context, "Insufficient buffer space available."); + return DC_STATUS_NOMEMORY; + }

nbytes += length; }

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 12/14] Cleanup: consistently check return value of iostream functions

Coverity CID 215197 Coverity CID 215200

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/citizen_aqualand.c | 12 ++++++++++-- src/hw_ostc.c | 14 ++++++++++++-- src/oceanic_atom2.c | 12 ++++++++++-- src/oceanic_vtpro.c | 10 +++++++--- src/reefnet_sensuspro.c | 12 ++++++++++-- src/suunto_d9.c | 12 ++++++++++-- src/suunto_solution.c | 6 +++++- src/suunto_vyper.c | 12 ++++++++++-- src/suunto_vyper2.c | 24 ++++++++++++++++++++---- 9 files changed, 94 insertions(+), 20 deletions(-)

diff --git a/src/citizen_aqualand.c b/src/citizen_aqualand.c index f639efdd9566..7510000b49fc 100644 --- a/src/citizen_aqualand.c +++ b/src/citizen_aqualand.c @@ -155,7 +155,11 @@ citizen_aqualand_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) // Erase the current contents of the buffer dc_buffer_clear (buffer);

- dc_iostream_set_dtr (device->iostream, 1); + status = dc_iostream_set_dtr (device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to set the DTR line."); + return status; + }

// Send the init byte. const unsigned char init[] = {0x7F}; @@ -200,7 +204,11 @@ citizen_aqualand_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) break; }

- dc_iostream_set_dtr (device->iostream, 0); + status = dc_iostream_set_dtr (device->iostream, 0); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to clear the DTR line."); + return status; + }

return DC_STATUS_SUCCESS; } diff --git a/src/hw_ostc.c b/src/hw_ostc.c index ed81fdf293df..05d511850e47 100644 --- a/src/hw_ostc.c +++ b/src/hw_ostc.c @@ -902,7 +902,12 @@ hw_ostc_device_fwupdate (dc_device_t *abstract, const char *filename) // bootloader needs to be send repeatedly, until the response packet is // received. Thus the time between each two attempts is directly controlled // by the timeout value. - dc_iostream_set_timeout (device->iostream, 300); + rc = dc_iostream_set_timeout (device->iostream, 300); + if (rc != DC_STATUS_SUCCESS) { + ERROR (context, "Failed to set the timeout."); + free (firmware); + return rc; + }

// Setup the bootloader. const unsigned int baudrates[] = {19200, 115200}; @@ -928,7 +933,12 @@ hw_ostc_device_fwupdate (dc_device_t *abstract, const char *filename) }

// Increase the timeout again. - dc_iostream_set_timeout (device->iostream, 1000); + rc = dc_iostream_set_timeout (device->iostream, 1000); + if (rc != DC_STATUS_SUCCESS) { + ERROR (context, "Failed to set the timeout."); + free (firmware); + return rc; + }

// Enable progress notifications. dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; diff --git a/src/oceanic_atom2.c b/src/oceanic_atom2.c index 77c8e9d54fdd..77b946ae44bf 100644 --- a/src/oceanic_atom2.c +++ b/src/oceanic_atom2.c @@ -630,8 +630,16 @@ oceanic_atom2_device_open (dc_device_t **out, dc_context_t *context, const char dc_iostream_sleep (device->iostream, 100);

// Set the DTR/RTS lines. - dc_iostream_set_dtr(device->iostream, 1); - dc_iostream_set_rts(device->iostream, 1); + status = dc_iostream_set_dtr(device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (context, "Failed to set the DTR line."); + return status; + } + status = dc_iostream_set_rts(device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (context, "Failed to set the DTR line."); + return status; + }

// Make sure everything is in a sane state. dc_iostream_purge (device->iostream, DC_DIRECTION_ALL); diff --git a/src/oceanic_vtpro.c b/src/oceanic_vtpro.c index cdb8943930b7..1ae4d4a79f04 100644 --- a/src/oceanic_vtpro.c +++ b/src/oceanic_vtpro.c @@ -269,9 +269,13 @@ oceanic_vtpro_calibrate (oceanic_vtpro_device_t *device) // device needs approximately 6 seconds to respond. unsigned char answer[2] = {0}; unsigned char command[2] = {0x18, 0x00}; - dc_iostream_set_timeout (device->iostream, 9000); - dc_status_t rc = oceanic_vtpro_transfer (device, command, sizeof (command), answer, sizeof (answer)); - dc_iostream_set_timeout (device->iostream, 3000); + dc_status_t rc = dc_iostream_set_timeout (device->iostream, 9000); + if (rc != DC_STATUS_SUCCESS) + return rc; + rc = oceanic_vtpro_transfer (device, command, sizeof (command), answer, sizeof (answer)); + if (rc != DC_STATUS_SUCCESS) + return rc; + rc = dc_iostream_set_timeout (device->iostream, 3000); if (rc != DC_STATUS_SUCCESS) return rc;

diff --git a/src/reefnet_sensuspro.c b/src/reefnet_sensuspro.c index 88c165028f12..2aee3ec40b29 100644 --- a/src/reefnet_sensuspro.c +++ b/src/reefnet_sensuspro.c @@ -182,7 +182,11 @@ reefnet_sensuspro_handshake (reefnet_sensuspro_device_t *device) dc_device_t *abstract = (dc_device_t *) device;

// Assert a break condition. - dc_iostream_set_break (device->iostream, 1); + status = dc_iostream_set_break (device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to set break."); + return status; + }

// Receive the handshake from the dive computer. unsigned char handshake[SZ_HANDSHAKE + 2] = {0}; @@ -193,7 +197,11 @@ reefnet_sensuspro_handshake (reefnet_sensuspro_device_t *device) }

// Clear the break condition again. - dc_iostream_set_break (device->iostream, 0); + status = dc_iostream_set_break (device->iostream, 0); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to clear break."); + return status; + }

// Verify the checksum of the handshake packet. unsigned short crc = array_uint16_le (handshake + SZ_HANDSHAKE); diff --git a/src/suunto_d9.c b/src/suunto_d9.c index 2c92a49c2a76..8c2834c8c799 100644 --- a/src/suunto_d9.c +++ b/src/suunto_d9.c @@ -240,7 +240,11 @@ suunto_d9_device_packet (dc_device_t *abstract, const unsigned char command[], u return DC_STATUS_CANCELLED;

// Clear RTS to send the command. - dc_iostream_set_rts (device->iostream, 0); + status = dc_iostream_set_rts (device->iostream, 0); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to clear RTS."); + return status; + }

// Send the command to the dive computer. status = dc_iostream_write (device->iostream, command, csize, NULL); @@ -265,7 +269,11 @@ suunto_d9_device_packet (dc_device_t *abstract, const unsigned char command[], u }

// Set RTS to receive the reply. - dc_iostream_set_rts (device->iostream, 1); + status = dc_iostream_set_rts (device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to set RTS."); + return status; + }

// Receive the answer of the dive computer. status = dc_iostream_read (device->iostream, answer, asize, NULL); diff --git a/src/suunto_solution.c b/src/suunto_solution.c index bbeaf8e90132..7242eed5f2cf 100644 --- a/src/suunto_solution.c +++ b/src/suunto_solution.c @@ -162,7 +162,11 @@ suunto_solution_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) unsigned char answer[3] = {0};

// Assert DTR - dc_iostream_set_dtr (device->iostream, 1); + status = dc_iostream_set_dtr(device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to set the DTR line."); + return status; + }

// Send: 0xFF command[0] = 0xFF; diff --git a/src/suunto_vyper.c b/src/suunto_vyper.c index 586a01716dfd..c82226b7ee91 100644 --- a/src/suunto_vyper.c +++ b/src/suunto_vyper.c @@ -178,7 +178,11 @@ suunto_vyper_send (suunto_vyper_device_t *device, const unsigned char command[], dc_iostream_sleep (device->iostream, 500);

// Set RTS to send the command. - dc_iostream_set_rts (device->iostream, 1); + status = dc_iostream_set_rts (device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to set RTS."); + return status; + }

// Send the command to the dive computer. status = dc_iostream_write (device->iostream, command, csize, NULL); @@ -202,7 +206,11 @@ suunto_vyper_send (suunto_vyper_device_t *device, const unsigned char command[], dc_iostream_purge (device->iostream, DC_DIRECTION_INPUT);

// Clear RTS to receive the reply. - dc_iostream_set_rts (device->iostream, 0); + status = dc_iostream_set_rts (device->iostream, 0); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to clear RTS."); + return status; + }

return DC_STATUS_SUCCESS; } diff --git a/src/suunto_vyper2.c b/src/suunto_vyper2.c index 6ddd3d7a7f42..25918784d2ee 100644 --- a/src/suunto_vyper2.c +++ b/src/suunto_vyper2.c @@ -127,10 +127,18 @@ suunto_vyper2_device_open (dc_device_t **out, dc_context_t *context, const char dc_iostream_sleep (device->iostream, 100);

// Make sure everything is in a sane state. - dc_iostream_purge (device->iostream, DC_DIRECTION_ALL); + status = dc_iostream_purge (device->iostream, DC_DIRECTION_ALL); + if (status != DC_STATUS_SUCCESS) { + ERROR (context, "Failed to reset IO state."); + goto error_close; + }

// Enable half-duplex emulation. - dc_iostream_set_halfduplex (device->iostream, 1); + status = dc_iostream_set_halfduplex (device->iostream, 1); + if (status != DC_STATUS_SUCCESS) { + ERROR (context, "Failed to set half duplex."); + goto error_close; + }

// Read the version info. status = suunto_common2_device_version ((dc_device_t *) device, device->base.version, sizeof (device->base.version)); @@ -187,7 +195,11 @@ suunto_vyper2_device_packet (dc_device_t *abstract, const unsigned char command[ dc_iostream_sleep (device->iostream, 600);

// Send the command to the dive computer. status = dc_iostream_write (device->iostream, command, csize, NULL); @@ -197,7 +209,11 @@ suunto_vyper2_device_packet (dc_device_t *abstract, const unsigned char command[ }

// Receive the answer of the dive computer. status = dc_iostream_read (device->iostream, answer, asize, NULL);

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 13/14] Cleanup: bail on error

Coverity CID 207809

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/suunto_eonsteel.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/src/suunto_eonsteel.c b/src/suunto_eonsteel.c index 948d1d329076..9b24840be3de 100644 --- a/src/suunto_eonsteel.c +++ b/src/suunto_eonsteel.c @@ -674,6 +674,7 @@ static int get_file_list(suunto_eonsteel_device_t *eon, struct directory_entry * sizeof(result), result); if (rc < 0) { ERROR(eon->base.context, "cmd DIR_LOOKUP failed"); + return -1; } HEXDUMP(eon->base.context, DC_LOGLEVEL_DEBUG, "DIR_LOOKUP", result, rc);

-- 2.15.1

Dirk Hohndel

7:35 p.m.

New subject: [PATCH 14/14] Cleanup: remove confusing NULL check

device has already been dereferenced before we ever get here

Coverity CID 207713 Coverity CID 207780

Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/mares_darwin.c | 2 +- src/mares_iconhd.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/mares_darwin.c b/src/mares_darwin.c index 308477cda6b6..e5716a9610a8 100644 --- a/src/mares_darwin.c +++ b/src/mares_darwin.c @@ -332,7 +332,7 @@ mares_darwin_extract_dives (dc_device_t *abstract, const unsigned char data[], u current -= length; }

- if (device && memcmp (buffer, device->fingerprint, sizeof (device->fingerprint)) == 0) { + if (memcmp (buffer, device->fingerprint, sizeof (device->fingerprint)) == 0) { free (buffer); return DC_STATUS_SUCCESS; } diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index b56da27a5b8a..90e49634cf19 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -610,7 +610,7 @@ mares_iconhd_device_foreach (dc_device_t *abstract, dc_dive_callback_t callback, break;

unsigned char *fp = buffer + offset + length - headersize + fingerprint; - if (device && memcmp (fp, device->fingerprint, sizeof (device->fingerprint)) == 0) { + if (memcmp (fp, device->fingerprint, sizeof (device->fingerprint)) == 0) { break; }

-- 2.15.1

Jef Driesen

4 Jan 4 Jan

1:58 p.m.

New subject: [PATCH 11/14] Cleanup: check error return values of buffer handling

On 2018-01-03 20:35, Dirk Hohndel wrote:

...

diff --git a/src/atomics_cobalt.c b/src/atomics_cobalt.c index a5ce98c9a302..5335eba94e47 100644 --- a/src/atomics_cobalt.c +++ b/src/atomics_cobalt.c @@ -297,7 +297,11 @@ atomics_cobalt_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, }
// Append the packet to the output buffer.
dc_buffer_append (buffer, packet, length);
if (!dc_buffer_append (buffer, packet, length)) {
	ERROR (abstract->context, "Insufficient buffer space available.");
	return DC_STATUS_NOMEMORY;
}
nbytes += length;

This one was already checked, although not immediately after the call. After the loop there is this check:

// Check for a buffer error. if (dc_buffer_get_size (buffer) != nbytes) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; }

The reason for that, is to make sure that the entire response is received, even if an error occurred. That avoids problems with "garbage" data left in the driver or OS input buffer on the next request. For the cobalt (and also the vyper) this matters because the protocol doesn't support re-downloading the same dive. So you can't retry on failure, like is done for other backends. You can only request the next dive (or start over from scratch). Note that this feature is not implemented at the moment, but the code has been written to support it if necessary.

...

diff --git a/src/divesystem_idive.c b/src/divesystem_idive.c index 642aaee4a05e..6fcdfabafc5c 100644 --- a/src/divesystem_idive.c +++ b/src/divesystem_idive.c @@ -491,8 +491,14 @@ divesystem_idive_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
dc_buffer_clear(buffer);
dc_buffer_reserve(buffer, commands->header.size +
commands->sample.size * nsamples);
dc_buffer_append(buffer, packet, commands->header.size);
if (!dc_buffer_reserve(buffer, commands->header.size +
commands->sample.size * nsamples)) {
	ERROR (abstract->context, "Insufficient buffer space available.");
	return DC_STATUS_NOMEMORY;
}
if (!dc_buffer_append(buffer, packet, commands->header.size)) {
	ERROR (abstract->context, "Insufficient buffer space available.");
	return DC_STATUS_NOMEMORY;
}
for (unsigned int j = 0; j < nsamples; j += commands->nsamples) { unsigned int idx = j + 1;
@@ -517,7 +523,10 @@ divesystem_idive_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb progress.current = i * NSTEPS + STEP(j + n + 1, nsamples + 1); device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
	dc_buffer_append(buffer, packet, commands->sample.size * n);
	if (!dc_buffer_append(buffer, packet, commands->sample.size * n)) {
		ERROR (abstract->context, "Insufficient buffer space available.");
		return DC_STATUS_NOMEMORY;
	}
}

unsigned char *data = dc_buffer_get_data(buffer);

Each of the three return statements introduces a memory leak.

...

diff --git a/src/hw_ostc.c b/src/hw_ostc.c index 4e4335caf291..ed81fdf293df 100644 --- a/src/hw_ostc.c +++ b/src/hw_ostc.c @@ -587,7 +587,10 @@ hw_ostc_device_screenshot (dc_device_t *abstract, dc_buffer_t *buffer, hw_ostc_f
if (format == HW_OSTC_FORMAT_RAW) {
	// Append the raw data to the output buffer.
	dc_buffer_append (buffer, raw, nbytes);
	if (!dc_buffer_append (buffer, raw, nbytes)) {
		ERROR (abstract->context, "Insufficient buffer space available.");
		return DC_STATUS_NOMEMORY;
	}
} else { // Store the decompressed data in the output buffer. for (unsigned int i = 0; i < count; ++i) {

Great, you caught a real bug here. For the raw format, the amount of memory reserved is just a guess, so the append can actually fail here.

...

diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index eca8dfbc30e8..15862c255443 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -1150,7 +1150,10 @@ hw_ostc3_firmware_readfile4 (dc_buffer_t *buffer, dc_context_t *context, const c size_t n = 0; unsigned char block[1024] = {0}; while ((n = fread (block, 1, sizeof (block), fp)) > 0) {
dc_buffer_append (buffer, block, n);
if (dc_buffer_append (buffer, block, n)) {
	ERROR (context, "Insufficient buffer space available.");
	return DC_STATUS_NOMEMORY;
}
}

// Close the file.

The file handle is leaked here!

...

diff --git a/src/mares_nemo.c b/src/mares_nemo.c index b3d262a5c18c..fc8a9a389d36 100644 --- a/src/mares_nemo.c +++ b/src/mares_nemo.c @@ -256,15 +256,24 @@ mares_nemo_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) ERROR (abstract->context, "Both packets are not equal."); return DC_STATUS_PROTOCOL; }
	dc_buffer_append (buffer, packet, PACKETSIZE);
	if (!dc_buffer_append (buffer, packet, PACKETSIZE)) {
		ERROR (abstract->context, "Insufficient buffer space available.");
		return DC_STATUS_NOMEMORY;
	}
} else if (crc1 == ccrc1) { // Only the first packet has a correct checksum. WARNING (abstract->context, "Only the first packet has a correct
checksum.");
	dc_buffer_append (buffer, packet, PACKETSIZE);
	if (!dc_buffer_append (buffer, packet, PACKETSIZE)) {
		ERROR (abstract->context, "Insufficient buffer space available.");
		return DC_STATUS_NOMEMORY;
	}
} else if (crc2 == ccrc2) { // Only the second packet has a correct checksum. WARNING (abstract->context, "Only the second packet has a correct
checksum.");
	dc_buffer_append (buffer, packet + PACKETSIZE + 1, PACKETSIZE);
	if (!dc_buffer_append (buffer, packet + PACKETSIZE + 1, 
PACKETSIZE)) {
		ERROR (abstract->context, "Insufficient buffer space available.");
		return DC_STATUS_NOMEMORY;
	}
} else { ERROR (abstract->context, "Unexpected answer checksum."); return DC_STATUS_PROTOCOL;

Just a minor remark: The amount of clutter could be reduced somewhat here, by moving the dc_buffer_append() call after the if statements:

if (!dc_buffer_append (buffer, packet + idx * (PACKETSIZE + 1), PACKETSIZE)) { ERROR (abstract->context, "Insufficient buffer space available."); return DC_STATUS_NOMEMORY; }

Inside the if statements you just need to set the idx variable to the correct packet (0 or 1).

...

@@ -863,7 +866,10 @@ suunto_eonsteel_device_foreach(dc_device_t *abstract, dc_dive_callback_t callbac // Reset the membuffer, put the 4-byte length at the head. dc_buffer_clear(file); put_le32(time, buf);
	dc_buffer_append(file, buf, 4);
	if (!dc_buffer_append(file, buf, 4)) {
		ERROR(abstract->context, "Insufficient buffer space available.");
		return DC_STATUS_NOMEMORY;
	}

// Then read the filename into the rest of the buffer
rc = read_file(eon, pathname, file);

The buffer is leaked here.

...

diff --git a/src/suunto_vyper.c b/src/suunto_vyper.c index cc4680bd7dbe..586a01716dfd 100644 --- a/src/suunto_vyper.c +++ b/src/suunto_vyper.c @@ -410,7 +410,10 @@ suunto_vyper_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, dc // transfer is finished. This approach leaves no data behind in // the serial receive buffer, and if this packet is part of the // last incomplete dive, no error has to be reported at all.
dc_buffer_append (buffer, answer + 2, len);
if (!dc_buffer_append (buffer, answer + 2, len)) {
	ERROR (abstract->context, "Insufficient buffer space available.");
	return DC_STATUS_NOMEMORY;
}
nbytes += len;

Same comment as for the cobalt. The buffer size is checked after the loop.

Jef

Jef Driesen

1:53 p.m.

New subject: [PATCH 09/14] Cleanup: dc_buffer_clear() should be a void function

On 2018-01-03 20:35, Dirk Hohndel wrote:

...

We sometimes ignored its return value, sometimes we didn't. Semantically, clearing a non-existing buffer is just a noop.

Careful - one side effect of the old semantic was that dc_buffer_clear() plus a bail on failure could be used to test if the buffer had been allocated. Right now there is at least one call site where we later call dc_buffer_apend() without checking its return value. This will be fixed in a later commit in this series.

Hmm, that's something I didn't think of. And there are two other potential side effects as well, that could cause us some trouble.

Before this patch, passing a NULL pointer would be detected immediately, before sending anything to the dive computer. After this patch, there are a few cases where the check is delayed. So that means we send some request to the dive computer, but might bail out before reading the response (or only a part of it). And that can easily cause a next request to fail as well, because the response data (or a part of it) remains in the driver or OS buffers.

For errors that we can detect early, like a NULL buffer, we should bail out as soon as possible. So instead of removing the dc_buffer_clear() based check, and postponing the check to some other buffer function further down, we should check it explicitly at the start of the function:

if (buffer == NULL) { ERROR (context, "No buffer space available."); return DC_STATUS_INVALIDARGS; }

(Note that I also replaced DC_STATUS_NOMEMORY with DC_STATUS_INVALIDARGS because that's probably a more suitable error code here.)

For the dump functions, which is the bulk of this patch, we can easily address all of them at once in the dc_device_dump function:

dc_status_t dc_device_dump (dc_device_t *device, dc_buffer_t *buffer) { if (device == NULL) return DC_STATUS_UNSUPPORTED;

if (device->vtable->dump == NULL) return DC_STATUS_UNSUPPORTED;

if (buffer == NULL) return DC_STATUS_INVALIDARGS;

return device->vtable->dump (device, buffer); }

Actually, also the dc_buffer_clear() call can be moved there. We always want to start with an empty buffer.

Another concern is that the dc_buffer_clear() function is part of the public api. So this change might break some applications. I don't mind doing this particular change, because it makes sense. But at the same time we also need to be careful that a simple fix to silence coverity doesn't turns into a problem for applications. (I don't think there are many applications using this function, but I don't know for sure.)

Jef

Dirk Hohndel

4:46 p.m.

New subject: [PATCH 09/14] Cleanup: dc_buffer_clear() should be a void function

On Thu, Jan 04, 2018 at 02:53:26PM +0100, Jef Driesen wrote:

...

Another concern is that the dc_buffer_clear() function is part of the public api. So this change might break some applications. I don't mind doing this particular change, because it makes sense. But at the same time we also need to be careful that a simple fix to silence coverity doesn't turns into a problem for applications. (I don't think there are many applications using this function, but I don't know for sure.)

Then let's just skip that whole patch from this series and figure out what you want to do regarding the API independent from this series. You are correct, a Coverity warning is a silly reason for a change that might be a lot more invasive in the end than initially considered.

Jef Driesen

1:45 p.m.

New subject: [PATCH 07/14] Cleanup: avoid undefined shift operation

On 2018-01-03 20:35, Dirk Hohndel wrote:

...

Shifting a 32bit value by 32 is undefined. Instead of using shifts to create the mask, explicitly create it by subtracting 1 from the signbit value (and using bitwise NOT to fill all the higher bits).

Coverity CID 207769

Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Dirk Hohndel dirk@hohndel.org

src/uwatec_smart_parser.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/uwatec_smart_parser.c b/src/uwatec_smart_parser.c index a70acb7b91c6..053cad9f2914 100644 --- a/src/uwatec_smart_parser.c +++ b/src/uwatec_smart_parser.c @@ -886,15 +886,14 @@ uwatec_smart_fixsignbit (unsigned int x, unsigned int n) return 0;

unsigned int signbit = (1 << (n - 1));
unsigned int mask = (0xFFFFFFFF << n);

// When turning a two's-complement number with a certain number // of bits into one with more bits, the sign bit must be repeated // in all the extra bits. if ((x & signbit) == signbit)
return x | mask;
return x | ~(signbit - 1);
else
return x & ~mask;
return x & (signbit - 1);
}

I would have kept the mask variable, like this:

unsigned int mask = (signbit - 1);

if ((x & signbit) == signbit) return x | ~mask; else return x & mask;

But the current version is fine too.

Jef

Jef Driesen

1:44 p.m.

New subject: [PATCH 06/14] Cleanup: ensure string is 0 terminated

On 2018-01-03 20:35, Dirk Hohndel wrote:

...

The Linux kernel uses the sir_name as a standard C string (in one instance copying it into a 60 char buffer using kstrncpy with a length limit of 60), we therefore need to ensure that it is 0 terminated.

Okay, in that case we should indeed null terminate the string.

If you look at a similar socket type, AF_UNIX, then the manpages says it's good practice to null-terminated the sun_path, but also that the linux kernel does support using up the entire buffer without a terminating null byte. This is all very much implementation dependent territory. AF_IRDA is probably just too obscure that no one bothered to handle this corner case.

...

Since the existing code didn't notify the caller if we were truncating the string at 25 characters, I didn't add such a warning/error for truncating at 24 characters.

Silent truncation is bad, so I think we should address this as well. But since that's a different issue, let's deal with it later, in a separate patch.

...

I was not able to find documentation on how Windows uses irdaServiceName so I didn't change that code.

Me neither. I suspect the size is specified somewhere in the IrDA protocol specification. And since the specification is the same for both platforms, I assume you should be able to pass service names with the same length on all platforms. So I'm leaning towards applying the same fix for Windows as well.

...

In both cases I replaced the hardcoded length of 25 with a sizeof() argument (but both Linux and Windows hard code that length in their headers, so it seems unlikely this would ever change).

Correct, but when reading the code now, you no longer need to know how large the buffer is. I consider that an improvement :-)

Jef

Jef Driesen

1:42 p.m.

New subject: [PATCH 05/14] Cleanup: avoid memory leak

On 2018-01-03 20:35, Dirk Hohndel wrote:

...

Coverity CID 207731

There are two memory leaks reported here. You forgot to fix the first one (the earlier divesystem_idive_transfer to read the header).

Jef

Linus Torvalds

3 Jan 3 Jan

8:07 p.m.

On Wed, Jan 3, 2018 at 11:35 AM, Dirk Hohndel dirk@hohndel.org wrote:

...

Linus, based on your feedback and Jef's comment I rewrote 7/14; can you please double check that I didn't mess this up by combining it all in one operation?

Looks ok to me. The rest looks fine too, although I notice that your git branch still has the old version.

Linus

Dirk Hohndel

8:17 p.m.

...

On Jan 3, 2018, at 12:07 PM, Linus Torvalds torvalds@linux-foundation.org wrote:

On Wed, Jan 3, 2018 at 11:35 AM, Dirk Hohndel dirk@hohndel.org wrote:

...
Linus, based on your feedback and Jef's comment I rewrote 7/14; can you please double check that I didn't mess this up by combining it all in one operation?

Looks ok to me. The rest looks fine too, although I notice that your git branch still has the old version.

I figured I'd wait to see how quickly they might land in Jef's tree - so while I referenced the last version in my coverity_scan branch, I hadn't pushed them to the actual Subsurface-branch (and not referenced them from Subsurface master).

I'll kick off a new Coverity build with that latest version in a moment.

Thanks

2683

Age (days ago)

2684

Last active (days ago)

devel@libdivecomputer.org

22 comments

3 participants

tags (0)

participants (3)

Dirk Hohndel
Jef Driesen
Linus Torvalds