Skip to content

fuzz: replace hardcoded numbers for bech32 limits #30596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
glozow merged 1 commit into from
Aug 6, 2024
Merged

fuzz: replace hardcoded numbers for bech32 limits #30596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/bech32.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ namespace

typedef std::vector<uint8_t> data;

/** The Bech32 and Bech32m checksum size */
constexpr size_t CHECKSUM_SIZE = 6;

/** The Bech32 and Bech32m character set for encoding. */
const char* CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l";

Expand Down
3 changes: 3 additions & 0 deletions src/bech32.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
namespace bech32
{

/** The Bech32 and Bech32m checksum size */
constexpr size_t CHECKSUM_SIZE = 6;
Comment on lines +24 to +25
Copy link
Contributor

@l0rinc l0rinc Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would it make sense to move these to the header as well - and make them static constexpr as well?

Suggested change
/** The Bech32 and Bech32m checksum size */
constexpr size_t CHECKSUM_SIZE = 6;
/** The Bech32 and Bech32m checksum size */
static constexpr size_t CHECKSUM_SIZE = 6;
/** The Bech32 and Bech32m character set for encoding. */
static constexpr const char* CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l";
/** The Bech32 and Bech32m character set for decoding. */
static constexpr const int8_t CHARSET_REV[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
15, -1, 10, 17, 21, 20, 26, 30, 7, 5, -1, -1, -1, -1, -1, -1,
-1, 29, -1, 24, 13, 25, 9, 8, 23, -1, 18, 22, 31, 27, 19, -1,
1, 0, 3, 16, 11, 28, 12, 14, 6, 4, 2, -1, -1, -1, -1, -1,
-1, 29, -1, 24, 13, 25, 9, 8, 23, -1, 18, 22, 31, 27, 19, -1,
1, 0, 3, 16, 11, 28, 12, 14, 6, 4, 2, -1, -1, -1, -1, -1
};

Copy link
Member Author

@josibake josibake Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, I don't think static makes a difference here since we aren't talking about a class member? Regarding CHARSET and CHARSET_REV, what's the advantage of having them in the header file if we aren't using them outside of bech32.cpp?

Copy link
Contributor

@l0rinc l0rinc Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think static makes a difference here

Based on other usages in the code, static constexpr in headers is good practice to prevent potential linking issues and ensure each translation unit has its own independent copy. Let me know if I'm wrong here.

what's the advantage of having them in the header file

Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.

If you think this is outside the scope, I can accept that, thanks for considering my suggestions.

Copy link
Member

@sipa sipa Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josibake The keyword static in C++ has two completely different meanings sadly 😠

Inside a class, a static variable/function makes it a class member rather than an instance member (shared by all instances).

Outside a class, static means that a variable/function is only accessible within its compilation unit (whose effect is identical to placing it in an anonymous namespace).

Copy link
Member Author

@josibake josibake Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sipa , @paplorinc thanks for the explanation! In this case, since we are in a named namespace, it seems the static keyword would be preferable? Or doesn't matter? Will do some more reading on this cause I'm not sure what the best practice is here.

Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.

This is another one I'm not sure about: in my mind, CHARSET is internal to bech32 hence why it makes sense to keep it in bech32.cpp. But I suppose you could also argue the char set is a definition (from BIP173), so it should be in the header. Either way, I think it's out of scope for this PR, but thought it was an interesting point.

Copy link
Contributor

@l0rinc l0rinc Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add this to #30623 (comment) as well?


enum class Encoding {
INVALID, //!< Failed decoding

Expand Down
5 changes: 3 additions & 2 deletions src/test/fuzz/bech32.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ FUZZ_TARGET(bech32)
std::vector<unsigned char> input;
ConvertBits<8, 5, true>([&](unsigned char c) { input.push_back(c); }, buffer.begin(), buffer.end());

if (input.size() + 3 + 6 <= 90) {
// If it's possible to encode input in Bech32(m) without exceeding the 90-character limit:
// Input data part + 3 characters for the HRP and separator (bc1) + the checksum characters
if (input.size() + 3 + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {
// If it's possible to encode input in Bech32(m) without exceeding the bech32-character limit:
for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) {
const std::string encoded = bech32::Encode(encoding, "bc", input);
Copy link
Contributor

@l0rinc l0rinc Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the bip the hrp MUST contain 1 to 83 US-ASCII characters - maybe we could extend the fuzz testing from the hard-coded "bc"

Copy link
Member Author

@josibake josibake Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that fuzzing for more than just the hard-coded "bc" is ideal, but probably better as a separate follow-up.

Copy link
Contributor

@l0rinc l0rinc Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you working on this or should I do it?

Copy link
Member Author

@josibake josibake Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up for grabs! Feel free to ping me for review.

Copy link
Contributor

@l0rinc l0rinc Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #30623 (comment)

assert(!encoded.empty());
Expand Down