refactor: Remove C-style const-violating cast, Use reinterpret

maflcko commented at 2:21 PM on July 22, 2023: member

Using a C-style cast to convert pointer types to a byte-like pointer type has many issues:

It may accidentally and silently throw away const.
It forces reviewers to check that it doesn't accidentally throw away const.

For example, on current master a const char* is cast to unsigned char* (without const), see https://github.com/bitcoin/bitcoin/blob/d23fda05842ba4539b225bbab01b94df0060f697/src/span.h#L273 . This can lead to UB, and the only reason why it didn't lead to UB is because the return type added back the const. (Obviously this would break if the return type was deduced via auto)

Fix all issues by adding back the const and using reinterpret_cast where appropriate.

DrahtBot commented at 2:21 PM on July 22, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, darosior, john-moffett
Concept ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

hebasto commented at 7:26 PM on July 22, 2023: member

Using a C-style cast to convert pointer types to a byte-like pointer type has many issues

Can clang-tidy or other tools be helpful in catching such cases in the future?

emc99 commented at 7:42 PM on July 22, 2023: none

Using a C-style cast to convert pointer types to a byte-like pointer type has many issues:

It may accidentally and silently throw away const.

It forces reviewers to check that it doesn't accidentally throw away const.

For example, on current master a const char* is cast to unsigned char* (without const), see

https://github.com/bitcoin/bitcoin/blob/d23fda05842ba4539b225bbab01b94df0060f697/src/span.h#L273

. This can lead to UB, and the only reason why it didn't lead to UB is because the return type added back the const. (Obviously this would break if the return type was deduced via auto) Fix all issues by adding back the const and using reinterpret_cast where appropriate.

What does 'UB' stand for? Something-Breakage?

sipa commented at 7:49 PM on July 22, 2023: member

UB = Undefined behavior

jonatack commented at 10:51 PM on July 23, 2023: member

Concept ACK

maflcko commented at 6:31 AM on July 24, 2023: member

Can clang-tidy or other tools be helpful in catching such cases in the future?

Yeah, you can use cppcoreguidelines-pro-type-cstyle-cast if you want. But that will produce some more warnings in places where a const is intentionally and correctly removed. Also, it will warn in some places that should use a Span instead. However, I think using Span in more places or other fixes can be left for follow-ups.

in src/crypto/common.h:20 in fa752d8b5d outdated

  18 |  
  19 |  uint16_t static inline ReadLE16(const unsigned char* ptr)
  20 |  {
  21 |      uint16_t x;
  22 | -    memcpy((char*)&x, ptr, 2);
  23 | +    memcpy(reinterpret_cast<char*>(&x), ptr, 2);

darosior commented at 9:53 AM on July 24, 2023:

Why casting in the first place since memcpy would reinterpret it as unsigned char * anyways?

hebasto commented at 10:29 AM on July 24, 2023: member

Concept ACK.

maflcko renamed this:
~~Remove C-style const-violating cast, Use reinterpret_cast~~
refactor: Remove C-style const-violating cast, Use reinterpret_cast
on Jul 24, 2023

DrahtBot added the label Refactoring on Jul 24, 2023

maflcko force-pushed on Jul 24, 2023

hebasto commented at 12:00 PM on July 24, 2023: member

UPD. Removed comment. nm

maflcko commented at 12:09 PM on July 24, 2023: member

Is this unused as well:

No? begin() returns a const iterator on the mutable keyChild. The cast is needed to remove the wrongly added const. As said previously, this is out-of-scope for this pull request: #28127 (comment)

hebasto approved

hebasto commented at 12:30 PM on July 24, 2023: member

ACK fa474bce4a9d47be04a03a3e36f25c5118eaf1aa, I have reviewed the code and it looks OK.

darosior approved

darosior commented at 12:40 PM on July 24, 2023: member

utACK fa474bce4a9d47be04a03a3e36f25c5118eaf1aa. This makes sense and looks good to me, though i'm far from a language lawyer.

nit: the first two commits share the same objective and title and could thus be squashed.

hebasto commented at 1:19 PM on July 24, 2023: member

As a follow up, I'm pretty sure that all casts of ArgsManager::ParseParameters's arguments in test/argsman_tests.cpp can be just dropped as well.

refactor: Remove unused C-style casts fa6394dd10

refactor: Avoid casting away constness

Seems confusing and brittle to remove const and then add it back in the
return type.

3333f950d4

refactor: Use reinterpret_cast where appropriate

Also, wrap reinterpret_cast into a CharCast to ensure it is only called
on byte pointers.

fa9108f85a

maflcko force-pushed on Jul 24, 2023

maflcko commented at 4:08 PM on July 24, 2023: member

Thanks, addressed both comments by reviewers.

hebasto approved

hebasto commented at 4:26 PM on July 24, 2023: member

re-ACK fa9108f85afdc926fd6a8b96cc2acff7ca25d7a8.

DrahtBot requested review from darosior on Jul 24, 2023

darosior commented at 4:28 PM on July 24, 2023: member

re-utACK fa9108f85afdc926fd6a8b96cc2acff7ca25d7a8

DrahtBot removed review request from darosior on Jul 24, 2023

fanquake requested review from john-moffett on Jul 26, 2023

john-moffett approved

john-moffett commented at 2:41 PM on July 26, 2023: contributor

ACK fa9108f85afdc926fd6a8b96cc2acff7ca25d7a8

fanquake merged this on Jul 26, 2023

fanquake closed this on Jul 26, 2023

maflcko deleted the branch on Jul 26, 2023

sidhujag referenced this in commit 30b966f4f7 on Aug 9, 2023

kwvg referenced this in commit d00f8a1316 on Nov 2, 2024

bitcoin locked this on Dec 5, 2024

refactor: Remove C-style const-violating cast, Use reinterpret_cast #28127

Reviews