Re: [PATCH] Coverity issue 434132

On 12/28/2012 08:49 AM, Kinkie wrote:

> the attached patch tries to address Coverity Scan issue 434132 -
> (Improper use of negative value (REVERSE_NEGATIVE).
>
> This is done by moving the check to the StoreIOBuffer constructor,
> making it more consitently enforced. It also changes the type of a
> data field from unsigned int to bool.
>
> What do you think?

> === modified file 'src/StoreIOBuffer.h'
> --- src/StoreIOBuffer.h 2012-09-01 14:38:36 +0000
> +++ src/StoreIOBuffer.h 2012-12-28 15:34:13 +0000
> @@ -45,7 +45,11 @@
>
> StoreIOBuffer(size_t aLength, int64_t anOffset, char *someData) :
> length (aLength), offset (anOffset), data (someData) {
> - flags.error = 0;
> + flags.error = false;
> + if (length < 0) {
> + flags.error = true;
> + length = 0;
> + }
> }
>

In a standard environment, aLength and, hence, length cannot be negative
in this context because aLength type is size_t. You will get warnings in
some environments after this change (and the change will not work).

> @@ -74,7 +78,7 @@
> struct {
> - unsigned error:1;
> + bool error:1;
> } flags;
> size_t length;
> int64_t offset;

I do not know exactly what problem you are trying to fix, but the 1-bit
StoreIOBuffer error field makes no sense to me: It does not save any
memory due to padding and probably makes computations more
CPU-expensive, not to mention possible coding bugs.

If you want to do just cosmetic changes and the error field is not
easily removed, then I suggest:

1. Replacing the whole flags struct with a single boolean error member.
2. Making three-parameter constructor private. Keep it logic-free but
assert that size is not negative (when converted to ssize_t).
3. Providing two static methods for creating erroneous (no parameters)
and successful (size, offset, and buffer parameters) I/O buffers.

Alternatively, one could investigate and improve the whole
store_client::callback() and error propagation interface. There may be a
better way to propagate changes. I do not recommend doing that if you
just want to fix a Coverity defect as it may turn out to be a complex
project and this defect alone is not a sufficient motivation for that.

HTH,

Alex.
Received on Fri Dec 28 2012 - 17:15:34 MST