Use constexpr to check memo validity #4249
Conversation
There was a problem hiding this comment.
Good call making that array constexpr. Nicely done. 👍
That said, I have four changes I'd like to suggest.
Three of those changes are specifically in the code you modified. If you want you can cherry-pick those change here: scottschurr@a6fcecf Also feel free to make the changes yourself if you prefer.
There's also a fourth change that should be made. I looked at the code coverage associated with your changes and noticed our current tests miss your changes all together. Not your fault! That was a pre-existing condition.
Nevertheless, since the code is being modified, it really ought to be exercised by the unit tests. So I cobbled together a small test that exercises your changes and a few other invalid memo conditions that were previously not exercised. A commit with those tests is here: scottschurr@2423686
You should feel free to cherry-pick that commit. Or, if you prefer, you can write your own test.
Thanks for your submission!
src/ripple/protocol/impl/STTx.cpp
Outdated
| @@ -226,7 +226,8 @@ STTx::checkSign( | |||
| return Unexpected("Internal signature check failure."); | |||
| } | |||
|
|
|||
| Json::Value STTx::getJson(JsonOptions) const | |||
| Json::Value | |||
There was a problem hiding this comment.
Yeah, I know in our header files the return type goes on its own line. However, for whatever reason, our clang-format settings want this moved to the same line as the rest of the declaration. Like
Json::Value STTx::getJson(JsonOptions) const
This must be fixed or else our continuous integration will fail.
src/ripple/protocol/impl/STTx.cpp
Outdated
| // The only allowed characters for MemoType and MemoFormat are the | ||
| // characters allowed in URLs per RFC 3986: alphanumerics and the | ||
| // following symbols: -._~:/?#[]@!$&'()*+,;=% | ||
| static constexpr std::array<char, 256> const allowedSymbols = []() { |
There was a problem hiding this comment.
Making this constexpr is a good call. Nicely done.
However, the std::array is only ever used in isMemoOkay(). The array can be static constexpr and also inside that function. Since the array is only ever used in that function I think it should be left inside `isMemoOkay(). That follows the principal of keeping things in the smallest possible scope.
There was a problem hiding this comment.
Agreed - this should stay within the isMemoOkay function to limit its scope as much as possible.
src/ripple/protocol/impl/STTx.cpp
Outdated
| "abcdefghijklmnopqrstuvwxyz"); | ||
|
|
||
| for (char c : symbols) | ||
| a[c] = c; |
There was a problem hiding this comment.
The previous implementation used...
a[c] = 1;
I think that is the more correct implementation.
Yes, you and I know that \0 is not currently one of the allowed characters. But, if it were, then the change you made would fail. Simply assigning a 1 means we don't have to worry about that corner case.
Here's a commit that addresses all three of the comments I have in this file: scottschurr@a6fcecf You can use that as an example. Or feel free to cherry-pick it as a [FOLD] commit. FWIW, if you go with the [FOLD], the final squashed commit will still have your name on it.
There was a problem hiding this comment.
This is a good point. When first looking at this, I noticed this change but didn't think much of it. I would have missed this.
Thank you @scottschurr; your comment makes clear why using a[c] = c is problem: it makes the \0 character appear to be "valid" when it isn't.
|
Thanks I have used your fix into my code |
There was a problem hiding this comment.
Changes look good! Thanks for doing this.
That said, I'm a bit uncomfortable that this change is not exercised by the unit tests. And, yeah, I have to admit that the code being untested is a long-standing problem. But just because it's a long-standing problem doesn't mean it shouldn't be addressed. I have two suggestions for how we can approach getting this code tested:
- You could add tests to this pull request or
- I could submit a pull request that adds tests for Memo.
I've written such tests, and your code passes them. You can see those tests here: https://github.com/scottschurr/rippled/commits/memo-test
You're welcome to add those tests (or similar) to this pull request. Or we can leave this pull request as it is and I'll submit a separate pull request that adds the tests to the code base. Either way the code base eventually has the same outcome.
Let me know which if those two approaches appeals more to you. Thanks.
For posterity: these tests were added by #4287 |
High Level Overview of Change
Context of Change
Type of Change