jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.22k stars 1.74k forks source link

Use assignment instead of memcpy to copy the value of a pointer #1078

Closed niooss-ledger closed 3 years ago

niooss-ledger commented 3 years ago

Some static code analyzers report warnings when sizeof is used on a pointer type, as this can be the sign of a missing dereference operation. For example clang's static analyzer has an alpha checker to report such issues, alpha.core.SizeofPtr.

In function allocate_memory, sizeof is used on a pointer:

void * base;
block *memory;

/* ... */

memcpy(&memory, &base, sizeof memory);

This triggers a warning, and this call to memcpy can safely be replaced by a usual assignment (the variables are not special in a way, they are aligned using the default alignment, etc.):

memory = base;
jedisct1 commented 3 years ago

Errr... copying a pointer is not the same as copying the data it contains.

And what is being copied here is a pointer, so sizeof of a pointer is intentional.

Static analyzers are useful, but provide hints rather than changes to be applied blindly.

jedisct1 commented 3 years ago

The block type is a uint8_t [], but this is not a requirement. There wouldn't be anything wrong with changing it to something having a different alignment. I'd rather keep memcpy() that makes it explicit that we are dealing with different pointer types.

niooss-ledger commented 3 years ago

Static analyzers are useful, but provide hints rather than changes to be applied blindly.

I agree, but when they find false-positive matches, it is useful to simplify the code in order to more easily get true-positive bugs from static analyzers.

The block type is a uint8_t []

Yes, even though this is not important. Here, what is copied is not the content of block, but the pointer block. And base has type void *, which enables copying it to any type without casting, but this is not important.

Errr... copying a pointer is not the same as copying the data it contains.

I did not understand your reply. Here, the code already copies the pointer. And in my humble opinion, using memcpy to copy a pointer can be confusing.

Moreover, from what I understand, the main difference between using memcpy(&memory, &base, sizeof memory); and memory = base; from the point of view of a compiler is that:

This would be a minor "optimization which reduces the stack usage of the program", but even if this is not so important, I tend to like programs which give some freedom to the compiler to perform optimizations, instead of hiding things with memcpy.

Anyway, if you believe that using memcpy(&memory, &base, sizeof memory); to copy a pointer is easier to maintain, I will not disagree.