Re: Avoid unecessary MemSet call (src/backend/utils/cache/relcache.c)

Em qua., 18 de mai. de 2022 às 05:54, Alvaro Herrera <
alvherre(at)alvh(dot)no-ip(dot)org> escreveu:

> This one caught my attention:
>
> diff --git a/contrib/pgcrypto/crypt-blowfish.c
> b/contrib/pgcrypto/crypt-blowfish.c
> index a663852ccf..63fcef562d 100644
> --- a/contrib/pgcrypto/crypt-blowfish.c
> +++ b/contrib/pgcrypto/crypt-blowfish.c
> @@ -750,7 +750,7 @@ _crypt_blowfish_rn(const char *key, const char
> *setting,
> /* Overwrite the most obvious sensitive data we have on the stack. Note
> * that this does not guarantee there's no sensitive data left on the
> * stack and/or in registers; I'm not aware of portable code that does. */
> - px_memset(&data, 0, sizeof(data));
> + px_memset(&data, 0, sizeof(struct data));
>
> return output;
> }
>
> The curious thing here is that sizeof(data) is correct, because it
> refers to a variable defined earlier in that function, whose type is an
> anonymous struct declared there. But I don't know what "struct data"
> refers to, precisely because that struct is unnamed. Am I misreading it?
>
No, you are right.
This is definitely wrong.

>
> Also:
>
> diff --git a/contrib/pgstattuple/pgstatindex.c
> b/contrib/pgstattuple/pgstatindex.c
> index e1048e47ff..87be62f023 100644
> --- a/contrib/pgstattuple/pgstatindex.c
> +++ b/contrib/pgstattuple/pgstatindex.c
> @@ -601,7 +601,7 @@ pgstathashindex(PG_FUNCTION_ARGS)
> errmsg("cannot access temporary indexes
> of other sessions")));
>
> /* Get the information we need from the metapage. */
> - memset(&stats, 0, sizeof(stats));
> + memset(&stats, 0, sizeof(HashIndexStat));
> metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ,
> LH_META_PAGE);
> metap = HashPageGetMeta(BufferGetPage(metabuf));
> stats.version = metap->hashm_version;
>
> I think the working theory here is that the original line is correct
> now, and it continues to be correct if somebody edits the function and
> makes variable 'stats' be of a different type. But if you change the
> sizeof() to use the type name, then there are two places that you need
> to edit, and they are not necessarily close together; so it is correct
> now and could become a bug in the future. I don't think we're fully
> consistent about this, but I think you're proposing to change it in the
> opposite direction that we'd prefer.
>
Yes. I think that only advantage using the name of structure is
when you read the line of MemSet, you know what kind type
is filled.

> For the case where the variable is a pointer, the developer could write
> 'sizeof(*variable)' instead of being forced to specify the type name,
> for example (just a random one):
>
Could have used this style to make the patch.
But the intention was to correct a possible misinterpretation,
which in this case, showed that I was totally wrong.

Sorry by the noise.

regards,
Ranier Vilela