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

maflcko · 2023-07-22T14:21:53Z

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

Line 273 in d23fda0

    
           inline const unsigned char* UCharCast(const char* c) { return (unsigned char*)c; }

. 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 · 2023-07-22T14:21:55Z

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 · 2023-07-22T19:26:24Z

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 · 2023-07-22T19:42:39Z

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

bitcoin/src/span.h

Line 273 in d23fda0

inline const unsigned char* UCharCast(const char* c) { return (unsigned char*)c; }

. 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 · 2023-07-22T19:49:05Z

UB = Undefined behavior

jonatack · 2023-07-23T22:51:46Z

Concept ACK

maflcko · 2023-07-24T06:31:29Z

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.

darosior · 2023-07-24T09:53:15Z

src/crypto/common.h


 #include <compat/endian.h>

 uint16_t static inline ReadLE16(const unsigned char* ptr)
 {
    uint16_t x;
-    memcpy((char*)&x, ptr, 2);
+    memcpy(reinterpret_cast<char*>(&x), ptr, 2);


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

hebasto · 2023-07-24T10:29:39Z

Concept ACK.

hebasto · 2023-07-24T12:00:20Z

UPD. Removed comment. nm

maflcko · 2023-07-24T12:09:38Z

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

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

darosior

utACK fa474bc. 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 · 2023-07-24T13:19:10Z

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.

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

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

maflcko · 2023-07-24T16:08:01Z

Thanks, addressed both comments by reviewers.

hebasto

re-ACK fa9108f.

darosior · 2023-07-24T16:28:34Z

re-utACK fa9108f

john-moffett

ACK fa9108f

…se reinterpret_cast fa9108f refactor: Use reinterpret_cast where appropriate (MarcoFalke) 3333f95 refactor: Avoid casting away constness (MarcoFalke) fa6394d refactor: Remove unused C-style casts (MarcoFalke) Pull request description: 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. ACKs for top commit: darosior: re-utACK fa9108f hebasto: re-ACK fa9108f. john-moffett: ACK fa9108f Tree-SHA512: 87f6e4b574f9bd96d4e0f2a0631fd0a9dc6096e5d4f1b95042fe9f197afc2fe9a24e333aeb34fed11feefcdb184a238fe1ea5aff10d580bb18d76bfe48b76a10

darosior reviewed Jul 24, 2023

View reviewed changes

maflcko changed the title ~~Remove C-style const-violating cast, Use reinterpret_cast~~ refactor: Remove C-style const-violating cast, Use reinterpret_cast Jul 24, 2023

DrahtBot added the Refactoring label Jul 24, 2023

maflcko force-pushed the 2307-no-const-cast- branch from fa752d8 to fa474bc Compare July 24, 2023 11:06

hebasto approved these changes Jul 24, 2023

View reviewed changes

darosior approved these changes Jul 24, 2023

View reviewed changes

MarcoFalke added 3 commits July 24, 2023 15:32

refactor: Remove unused C-style casts

fa6394d

refactor: Avoid casting away constness

3333f95

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

refactor: Use reinterpret_cast where appropriate

fa9108f

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

maflcko force-pushed the 2307-no-const-cast- branch from fa474bc to fa9108f Compare July 24, 2023 13:34

hebasto approved these changes Jul 24, 2023

View reviewed changes

DrahtBot requested a review from darosior July 24, 2023 16:26

DrahtBot removed the request for review from darosior July 24, 2023 16:28

fanquake requested a review from john-moffett July 26, 2023 13:26

john-moffett approved these changes Jul 26, 2023

View reviewed changes

fanquake merged commit f57e724 into bitcoin:master Jul 26, 2023
15 checks passed

maflcko deleted the 2307-no-const-cast- branch July 26, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maflcko commented Jul 22, 2023

DrahtBot commented Jul 22, 2023 •

edited

hebasto commented Jul 22, 2023

emc99 commented Jul 22, 2023

sipa commented Jul 22, 2023

jonatack commented Jul 23, 2023

maflcko commented Jul 24, 2023

darosior Jul 24, 2023

hebasto commented Jul 24, 2023

hebasto commented Jul 24, 2023 •

edited

maflcko commented Jul 24, 2023

hebasto left a comment

darosior left a comment

hebasto commented Jul 24, 2023

maflcko commented Jul 24, 2023

hebasto left a comment

darosior commented Jul 24, 2023

john-moffett left a comment

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

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

Conversation

maflcko commented Jul 22, 2023

DrahtBot commented Jul 22, 2023 • edited

Reviews

hebasto commented Jul 22, 2023

emc99 commented Jul 22, 2023

sipa commented Jul 22, 2023

jonatack commented Jul 23, 2023

maflcko commented Jul 24, 2023

darosior Jul 24, 2023

Choose a reason for hiding this comment

hebasto commented Jul 24, 2023

hebasto commented Jul 24, 2023 • edited

maflcko commented Jul 24, 2023

hebasto left a comment

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

hebasto commented Jul 24, 2023

maflcko commented Jul 24, 2023

hebasto left a comment

Choose a reason for hiding this comment

darosior commented Jul 24, 2023

john-moffett left a comment

Choose a reason for hiding this comment

DrahtBot commented Jul 22, 2023 •

edited

hebasto commented Jul 24, 2023 •

edited