[PATCH 11/14] Cleanup: check error return values of buffer handling
Jef Driesen
jef at libdivecomputer.org
Thu Jan 4 05:58:01 PST 2018
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
More information about the devel
mailing list