jedisct1 / libsodium

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

Define randombytes_set_implementation argument to be const #1063

Closed jwf closed 3 years ago

jwf commented 3 years ago

Discourage developers from creating writable function pointers. If someone insists on having their implementation spec be writable, it's easy to cast to const, but if someone does the right thing and defines it as a const, it's harder to go the other way.

jedisct1 commented 3 years ago

If my memory serves me correctly, this was intentional, due to some compilers not supporting constant function pointers.

jedisct1 commented 3 years ago

There wouldn't be anything wrong with passing a function pointer and change it later. I can see applications doing this intentionally, hardly by accident.

So, I'd rather avoid a breaking change here

cemeyer commented 3 years ago

If my memory serves me correctly, this was intentional, due to some compilers not supporting constant function pointers.

If we need to support broken compilers, we could conditionally define a macro that evaluates to const on conventional compilers and to nothing on broken compilers. Would that be an ok workaround for this concern? (Which compilers do we support that have this deficiency?)

There wouldn't be anything wrong with passing a function pointer and change it later. I can see applications doing this intentionally, hardly by accident.

So, I'd rather avoid a breaking change here

const in this position doesn't prevent applications from defining and modifying non-const vtables; it just promises that libsodium won't modify the vtable via the application-provided pointer. I don't believe this is a breaking change for applications using non-const storage for this vtable.

Edit: I think the commit message is a little unclear and perhaps lead to these concerns:

Discourage developers from creating writable function pointers. If someone insists on having their implementation spec be writable, it's easy to cast to const, but if someone does the right thing and defines it as a const, it's harder to go the other way.

Maybe better?

Allow applications to use const vtables. The promise here is that libsodium will not modify the application's vtable. Of course, libsodium does not and will not modify the application provided vtable, so this is just being explicit about an existing property. Applications providing non-const vtable pointers will have those pointers implicitly coerced to const in the function invocation, without any code changes in the application.

(Without this change, applications with const vtables must use kludges like casting-through-uintptr_t to strip the const qualifier from the vtable pointer.)

jedisct1 commented 3 years ago

Definitely better with the new commit message!