This is 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).
Some of them are clear bug fixes, some of them are equally clear fixes to avoid memory leaks. A few deal with cases where the current code ignores the return values of functions - that last type I am not always 100% sure about my resolution...
All of them were generated based on a Coverity scan of Subsurface, built on Linux. This series should address all of the currently open Coverity issues that aren't rather obvious false positives.
As usual, one can argue whether all of these issues are indeed bugs and how they should be fixed (e.g., should we check return values and return errors to our callers, or is it safe to ignore certain errors (in which case that should be done explicitly for consistency)).
Happy to get feedback / comments.
Thanks
/D
[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: be explicit about ignoring the return value of [PATCH 10/14] Cleanup: check return value of ioctl() [PATCH 11/14] Cleanup: check error return values of buffer handling [PATCH 12/14] Cleanup: consistenty check return value of iostream [PATCH 13/14] Cleanup: bail on error [PATCH 14/14] Cleanup: remove confusing NULL check
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 {
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; }
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); }
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; }
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
Coverity CID 207790
Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/irda.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/irda.c b/src/irda.c index 149808aaa5c2..2964d4d8cb3a 100644 --- a/src/irda.c +++ b/src/irda.c @@ -229,10 +229,12 @@ dc_irda_connect_name (dc_iostream_t *abstract, unsigned int address, const char struct sockaddr_irda peer; peer.sir_family = AF_IRDA; peer.sir_addr = address; - if (name) + if (name) { strncpy (peer.sir_name, name, 25); - else + peer.sir_name[24] = '\0'; + } else { memset (peer.sir_name, 0x00, 25); + } #endif
return dc_socket_connect (&device->base, (struct sockaddr *) &peer, sizeof (peer));
Coverity CID 207769
Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/uwatec_smart_parser.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/uwatec_smart_parser.c b/src/uwatec_smart_parser.c index a70acb7b91c6..acbb6eb8d983 100644 --- a/src/uwatec_smart_parser.c +++ b/src/uwatec_smart_parser.c @@ -886,7 +886,13 @@ uwatec_smart_fixsignbit (unsigned int x, unsigned int n) return 0;
unsigned int signbit = (1 << (n - 1)); - unsigned int mask = (0xFFFFFFFF << n); + unsigned int mask; + + // shifting a 32bit constant by more than 31 bits has undefined behavior + if (n == 32) + mask = 0; + else + 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
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); }
Coverity CID 207756 Coverity CID 207714 Coverity CID 207800 Coverity CID 207729
Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/atomics_cobalt.c | 2 +- src/divesystem_idive.c | 2 +- src/oceanic_vtpro.c | 2 +- src/suunto_eonsteel.c | 2 +- src/suunto_vyper.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/atomics_cobalt.c b/src/atomics_cobalt.c index 7c7dccb04876..3b42147197a4 100644 --- a/src/atomics_cobalt.c +++ b/src/atomics_cobalt.c @@ -323,7 +323,7 @@ atomics_cobalt_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, // When only two 0xFF bytes are received, there are no more dives. unsigned char *data = dc_buffer_get_data (buffer); if (nbytes == 2 && data[0] == 0xFF && data[1] == 0xFF) { - dc_buffer_clear (buffer); + (void)dc_buffer_clear (buffer); return DC_STATUS_SUCCESS; }
diff --git a/src/divesystem_idive.c b/src/divesystem_idive.c index 642aaee4a05e..138aac9fb526 100644 --- a/src/divesystem_idive.c +++ b/src/divesystem_idive.c @@ -490,7 +490,7 @@ divesystem_idive_device_foreach (dc_device_t *abstract, dc_dive_callback_t callb progress.current = i * NSTEPS + STEP(1, nsamples + 1); device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
- dc_buffer_clear(buffer); + (void)dc_buffer_clear(buffer); dc_buffer_reserve(buffer, commands->header.size + commands->sample.size * nsamples); dc_buffer_append(buffer, packet, commands->header.size);
diff --git a/src/oceanic_vtpro.c b/src/oceanic_vtpro.c index e1ba31ced88a..f200b78fe7cc 100644 --- a/src/oceanic_vtpro.c +++ b/src/oceanic_vtpro.c @@ -363,7 +363,7 @@ oceanic_aeris500ai_device_logbook (dc_device_t *abstract, dc_event_progress_t *p
// Compare the fingerprint to identify previously downloaded entries. if (memcmp (answer, device->base.fingerprint, PAGESIZE / 2) == 0) { - dc_buffer_clear (logbook); + (void)dc_buffer_clear (logbook); } else { dc_buffer_append (logbook, answer, PAGESIZE / 2); } diff --git a/src/suunto_eonsteel.c b/src/suunto_eonsteel.c index cbf1b756d945..cddc8611abe8 100644 --- a/src/suunto_eonsteel.c +++ b/src/suunto_eonsteel.c @@ -861,7 +861,7 @@ suunto_eonsteel_device_foreach(dc_device_t *abstract, dc_dive_callback_t callbac break;
// Reset the membuffer, put the 4-byte length at the head. - dc_buffer_clear(file); + (void)dc_buffer_clear(file); put_le32(time, buf); dc_buffer_append(file, buf, 4);
diff --git a/src/suunto_vyper.c b/src/suunto_vyper.c index ba5ad20047b7..a59b75953101 100644 --- a/src/suunto_vyper.c +++ b/src/suunto_vyper.c @@ -396,7 +396,7 @@ suunto_vyper_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, dc // the current dive has been overwritten with newer data. Therefore, // we discard the current (incomplete) dive and end the transmission. if (len == 0) { - dc_buffer_clear (buffer); + (void)dc_buffer_clear (buffer); return DC_STATUS_SUCCESS; }
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.
Coverity CID 207798
Signed-off-by: Dirk Hohndel dirk@hohndel.org --- src/divesystem_idive.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/divesystem_idive.c b/src/divesystem_idive.c index 138aac9fb526..41b8260ff0c4 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);
(void)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;
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 3af5d528e863..4d1817ceb801 100644 --- a/src/citizen_aqualand.c +++ b/src/citizen_aqualand.c @@ -159,7 +159,11 @@ citizen_aqualand_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return DC_STATUS_NOMEMORY; }
- 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}; @@ -201,7 +205,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 36d608086e96..89673931afd1 100644 --- a/src/hw_ostc.c +++ b/src/hw_ostc.c @@ -905,7 +905,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}; @@ -931,7 +936,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 f200b78fe7cc..3b0884a14aa4 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 e50cb44937f1..5af91812b367 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 2e9e14efa707..ffa8262b03bb 100644 --- a/src/suunto_solution.c +++ b/src/suunto_solution.c @@ -161,7 +161,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 a59b75953101..c04b775a645c 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);
// 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 the RTS line."); + return status; + }
// 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[ }
// 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 set the RTS line."); + return status; + }
// Receive the answer of the dive computer. status = dc_iostream_read (device->iostream, answer, asize, NULL);
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 cddc8611abe8..78b91f7798d1 100644 --- a/src/suunto_eonsteel.c +++ b/src/suunto_eonsteel.c @@ -671,6 +671,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);
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 4f87f6fcd14a..e4d118fac4f8 100644 --- a/src/mares_darwin.c +++ b/src/mares_darwin.c @@ -331,7 +331,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 990f46657bdb..bc7f5d97be99 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -609,7 +609,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; }
On 29-12-17 01:35, Dirk Hohndel wrote:
--- 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;
This fix changes the behavior slightly, but that's no longer an issue now.
The original intention was that the timeout is always restored, even if an error occurred. In the past this was important because the function was exposed in the public api and the connection could be re-used after a failed call. Nowadays, this is no longer the case, because it's only called internally, and at that point any error is fatal (e.g. no handle is returned to the caller, and thus re-using it is simply not possible).
Jef
On Jan 3, 2018, at 7:34 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 29-12-17 01:35, Dirk Hohndel wrote:
--- 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;
This fix changes the behavior slightly, but that's no longer an issue now.
The original intention was that the timeout is always restored, even if an error occurred. In the past this was important because the function was exposed in the public api and the connection could be re-used after a failed call. Nowadays, this is no longer the case, because it's only called internally, and at that point any error is fatal (e.g. no handle is returned to the caller, and thus re-using it is simply not possible).
Ouch, I missed that part. It would be easy to fix that - simply to make sure that we aren't causing unintended side effects.
/D
On 29-12-17 01:35, Dirk Hohndel wrote:
Coverity CID 207798
Signed-off-by: Dirk Hohndel dirk@hohndel.org
src/divesystem_idive.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/divesystem_idive.c b/src/divesystem_idive.c index 138aac9fb526..41b8260ff0c4 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); (void)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;
The dc_buffer_reserve() call should indeed be checked, because the underlying memory allocation can fail. But for the dc_buffer_append() it's a bit pointless. Once the memory is reserved it can't fail anymore (unless you pass invalid arguments).
I'm surprised coverity complains about this dc_buffer_append() call, but not about the next dc_buffer_append() call a bit further down. If we add the error checking, then I think we should do it for both calls.
Jef
On Jan 3, 2018, at 7:32 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 29-12-17 01:35, Dirk Hohndel wrote:
Coverity CID 207798 Signed-off-by: Dirk Hohndel dirk@hohndel.org
src/divesystem_idive.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/divesystem_idive.c b/src/divesystem_idive.c index 138aac9fb526..41b8260ff0c4 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); (void)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;The dc_buffer_reserve() call should indeed be checked, because the underlying memory allocation can fail. But for the dc_buffer_append() it's a bit pointless. Once the memory is reserved it can't fail anymore (unless you pass invalid arguments).
This is one of those issues with static analysis. Sometimes it is surprisingly clever in figuring things like this out. And sometimes it misses obvious things.
I'm surprised coverity complains about this dc_buffer_append() call, but not about the next dc_buffer_append() call a bit further down. If we add the error checking, then I think we should do it for both calls.
Actually, we had a couple such instances where Coverity would flag one of two more or less identical scenarios in the same file or even the same function. That confused me as well.
I'm happy to do either: add return checks to all callers of dc_buffer_append() or mark the cases where we reserve the memory and simply use that reserved memory as false positives.
I'm slightly leaning towards the former if you don't care either way, simply because that prevents copy and paste errors (someone grabs a code snippet and reuses it, but doesn't include the dc_buffer_reserve() call in the snippet). I don't think we are so concerned about code size or runtime that this additional (and in this case, guaranteed to succeed) check would be a problem. But it's your call.
/D
On 29-12-17 01:35, Dirk Hohndel wrote:
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
I didn't check this one, because it's not the most useful one. But it also doesn't really hurt, so I'm fine with it either way.
Note that the error code returned by the dc_serial_close() function is mainly for informative purposes anyway. The caller can't really do much with it other then reporting it.
Jef
On Jan 3, 2018, at 7:31 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 29-12-17 01:35, Dirk Hohndel wrote:
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
I didn't check this one, because it's not the most useful one. But it also doesn't really hurt, so I'm fine with it either way.
Note that the error code returned by the dc_serial_close() function is mainly for informative purposes anyway. The caller can't really do much with it other then reporting it.
I'm fine either way - we can always mark this as intentional in Coverity (btw, you haven't asked for access to the scan reports - can you see the reports without me giving you access after all?)
I simply added these checks because in general libdivecomputer is extremely consistent and conservative with error checking, so this did seem like an unintentional omission to me.
/D
On 2018-01-03 16:39, Dirk Hohndel wrote:
On Jan 3, 2018, at 7:31 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 29-12-17 01:35, Dirk Hohndel wrote:
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
I didn't check this one, because it's not the most useful one. But it also doesn't really hurt, so I'm fine with it either way.
Note that the error code returned by the dc_serial_close() function is mainly for informative purposes anyway. The caller can't really do much with it other then reporting it.
I'm fine either way - we can always mark this as intentional in Coverity (btw, you haven't asked for access to the scan reports - can you see the reports without me giving you access after all?)
I haven't checked yet, but I certainly will because I'm sure it's useful to see what coverity actually reports for all these issues. But I need to leave in a few minutes, and wanted to send my comments as soon as possible :-)
I simply added these checks because in general libdivecomputer is extremely consistent and conservative with error checking, so this did seem like an unintentional omission to me.
It causes no harm, so let's just add the check.
Jef
On 29-12-17 01:35, Dirk Hohndel wrote:
--- a/src/atomics_cobalt.c +++ b/src/atomics_cobalt.c @@ -323,7 +323,7 @@ atomics_cobalt_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, // When only two 0xFF bytes are received, there are no more dives. unsigned char *data = dc_buffer_get_data (buffer); if (nbytes == 2 && data[0] == 0xFF && data[1] == 0xFF) {
dc_buffer_clear (buffer);
(void)dc_buffer_clear (buffer);
Hmm, I'm not really a fan of this. It adds more visual noise for very little real gain (other than silencing the static analyzer).
Maybe this function should have returned a void instead.
Jef
On Jan 3, 2018, at 7:27 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 29-12-17 01:35, Dirk Hohndel wrote:
--- a/src/atomics_cobalt.c +++ b/src/atomics_cobalt.c @@ -323,7 +323,7 @@ atomics_cobalt_read_dive (dc_device_t *abstract, dc_buffer_t *buffer, int init, // When only two 0xFF bytes are received, there are no more dives. unsigned char *data = dc_buffer_get_data (buffer); if (nbytes == 2 && data[0] == 0xFF && data[1] == 0xFF) {
dc_buffer_clear (buffer);
(void)dc_buffer_clear (buffer);Hmm, I'm not really a fan of this. It adds more visual noise for very little real gain (other than silencing the static analyzer).
This is something that we have struggled with in Subsurface. We have generally decided to go with explicitly marking things as void that aren't used, but especially in this circumstance I agree that this is just ugly and distracting.
Maybe this function should have returned a void instead.
That had been my initial thought as well. If it is passed a NULL pointer, then clearing the buffer is simply a NOP...
If you would like me to do that, I'm happy to rework the patch to do that instead and update the other callers.
/D
On Thu, Dec 28, 2017 at 4:35 PM, Dirk Hohndel dirk@hohndel.org wrote:
unsigned int signbit = (1 << (n - 1));
unsigned int mask = (0xFFFFFFFF << n);
unsigned int mask;// shifting a 32bit constant by more than 31 bits has undefined behaviorif (n == 32)mask = 0;elsemask = (0xFFFFFFFF << n);
Ugh. That's ugly.
It's much simpler to just do
unsigned int mask = ~(signbit-1);
instead.
Yes, that mask ends up including the sign bit (the old mask did not), but that doesn't matter for the end result.
Linus
On 29-12-17 08:00, Linus Torvalds wrote:
On Thu, Dec 28, 2017 at 4:35 PM, Dirk Hohndel dirk@hohndel.org wrote:
unsigned int signbit = (1 << (n - 1));
unsigned int mask = (0xFFFFFFFF << n);
unsigned int mask;// shifting a 32bit constant by more than 31 bits hasundefined behavior
if (n == 32)mask = 0;elsemask = (0xFFFFFFFF << n);Ugh. That's ugly.
It's much simpler to just do
unsigned int mask = ~(signbit-1);instead.
Yes, that mask ends up including the sign bit (the old mask did not), but that doesn't matter for the end result.
That's indeed an easier and nicer fix!
The bitwise not can even be omitted from the mask, and moved to the return statements (where there is already both a mask and a ~mask).
Jef
On Jan 3, 2018, at 7:23 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 29-12-17 08:00, Linus Torvalds wrote:
On Thu, Dec 28, 2017 at 4:35 PM, Dirk Hohndel dirk@hohndel.org wrote:
unsigned int signbit = (1 << (n - 1));
unsigned int mask = (0xFFFFFFFF << n);
unsigned int mask;// shifting a 32bit constant by more than 31 bits has undefined behaviorif (n == 32)mask = 0;elsemask = (0xFFFFFFFF << n);Ugh. That's ugly. It's much simpler to just do unsigned int mask = ~(signbit-1); instead. Yes, that mask ends up including the sign bit (the old mask did not), but that doesn't matter for the end result.
That's indeed an easier and nicer fix!
The bitwise not can even be omitted from the mask, and moved to the return statements (where there is already both a mask and a ~mask).
I will update accordingly.
/D
On 29-12-17 01:35, Dirk Hohndel wrote:
Coverity CID 207790
Signed-off-by: Dirk Hohndel dirk@hohndel.org
src/irda.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/irda.c b/src/irda.c index 149808aaa5c2..2964d4d8cb3a 100644 --- a/src/irda.c +++ b/src/irda.c @@ -229,10 +229,12 @@ dc_irda_connect_name (dc_iostream_t *abstract, unsigned int address, const char struct sockaddr_irda peer; peer.sir_family = AF_IRDA; peer.sir_addr = address;
- if (name)
- if (name) { strncpy (peer.sir_name, name, 25);
- else
peer.sir_name[24] = '\0';- } else { memset (peer.sir_name, 0x00, 25);
- } #endif
For this one, I'm not sure whether your fix is the right thing to do. If the sir_name field is used as a fixed size byte array (possibly padded with zero's) instead of a null terminated string, then null terminating it may silently truncate the last byte and cause problems. I don't know how the irda stack uses this field, so I simply don't know the right answer here.
Checking for possible truncation, and returning an error if the string doesn't fit, is probably a safer solution. Even without this patch, truncation is already a problem.
PS: Using sizeof instead of hardcoding the length to 25 would be a good idea as well.
Jef
On Jan 3, 2018, at 7:23 AM, Jef Driesen jef@libdivecomputer.org wrote:
On 29-12-17 01:35, Dirk Hohndel wrote:
Coverity CID 207790 Signed-off-by: Dirk Hohndel dirk@hohndel.org
src/irda.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/irda.c b/src/irda.c index 149808aaa5c2..2964d4d8cb3a 100644 --- a/src/irda.c +++ b/src/irda.c @@ -229,10 +229,12 @@ dc_irda_connect_name (dc_iostream_t *abstract, unsigned int address, const char struct sockaddr_irda peer; peer.sir_family = AF_IRDA; peer.sir_addr = address;
- if (name)
- if (name) { strncpy (peer.sir_name, name, 25);
- else
peer.sir_name[24] = '\0';- } else { memset (peer.sir_name, 0x00, 25);
- }
#endif
For this one, I'm not sure whether your fix is the right thing to do. If the sir_name field is used as a fixed size byte array (possibly padded with zero's) instead of a null terminated string, then null terminating it may silently truncate the last byte and cause problems. I don't know how the irda stack uses this field, so I simply don't know the right answer here.
That's the same worry I had.
Checking for possible truncation, and returning an error if the string doesn't fit, is probably a safer solution. Even without this patch, truncation is already a problem.
PS: Using sizeof instead of hardcoding the length to 25 would be a good idea as well.
I'll do some more research on what the IRDA stack expects here and come up with a better fix.
Thanks for the review
/D
-
Dirk Hohndel -
Jef Driesen -
Linus Torvalds