At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in > Hi Hackers, > > Per Coverity. > > Coverity complaints about pg_cryptohash_final function. > And I agree with Coverity, it's a bad design. > Its allows this: > > #define MY_RESULT_LENGTH 32 > > function pgtest(char * buffer, char * text) { > pg_cryptohash_ctx *ctx; > uint8 digest[MY_RESULT_LENGTH]; > > ctx = pg_cryptohash_create(PG_SHA512); > pg_cryptohash_init(ctx); > pg_cryptohash_update(ctx, (uint8 *) buffer, text); > pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1): > Out-of-bounds access (OVERRUN) > pg_cryptohash_free(ctx); > return > }
It seems to me that the above just means the caller must provide a digest buffer that fits the use. In the above example digest just must be 64 byte. If Coverity complains so, what should do for the complaint is to fix the caller to provide a digest buffer of the correct size.
Exactly.
Could you show the detailed context where Coverity complained?
Coverity complains about call memcpy with fixed size, in a context with buffer variable size supplied by the caller.
> Attached has a patch with suggestions to make things better.
So it doesn't seem to me the right direction. Even if we are going to make pg_cryptohash_final to take the buffer length, it should error-out or assert-out if the length is too small rather than copy a part of the digest bytes. (In short, it would only be assertion-use.)
It is necessary to correct the interfaces. To caller, inform the size of the buffer it created.
I think it should be error-out, because the buffer can be malloc.