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

[Dirk Hohndel]

> On Jan 3, 2018, at 7:32 AM, Jef Driesen <jef at libdivecomputer.org> wrote:
> 
> On 29-12-17 01:35, Dirk Hohndel wrote:
>> Coverity CID 207798
>> Signed-off-by: Dirk Hohndel <dirk at 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