Skip to content
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.

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

Sanitize uacomment #6647

Merged
merged 1 commit into from Sep 22, 2015
Merged

Conversation

@MarcoFalke
Copy link
Member

MarcoFalke commented Sep 7, 2015

  • SanitizeString() can be requested to be more strict
  • Actually apply SanitizeString() to uacomments
@jonasschnelli
jonasschnelli reviewed Sep 7, 2015
View changes
src/utilstrencodings.h Outdated
@@ -22,7 +22,15 @@
/** This is needed because the foreach macro can't get over the comma in pair<t1, t2> */
#define PAIRTYPE(t1, t2) std::pair<t1, t2>

std::string SanitizeString(const std::string& str);
static const int SAFE_CHARS_DEFAULT = 0; //!< Default rule in SanitizeString()

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Sep 7, 2015

Member

nit: would a enum not be more adequate for a such operation?

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 8, 2015

Member

+1 for using enum. This is a perfect fit for one.

Not sure passing in the characters in a string is better. The idea makes sense, but having it like this keeps open the possibility of optimizing SanitizeString to use something else than string.find() on every character.

@jonasschnelli
jonasschnelli reviewed Sep 7, 2015
View changes
src/utilstrencodings.cpp Outdated
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890 .,;_?@" // SAFE_CHARS_UACOMMENT
};

string SanitizeString(const string& str, int rule)

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Sep 7, 2015

Member

Instead of the slightly static rule parameter, would a charset represented as string with additional forbidden chars not be more flexible.
Instead SanitizeString("Comment2 .,_?@", SAFE_CHARS_UACOMMENT) it would then be SanitizeString("Comment2 .,_?@", "/:()").
Not sure if we wan't to have use case specific handling within SanitizeString().

@jonasschnelli
Copy link
Member

jonasschnelli commented Sep 7, 2015

Concept ACK.

strSubVersion = FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, mapMultiArgs.count("-uacomment") ? mapMultiArgs["-uacomment"] : std::vector<string>());
// sanitize comments per BIP-0014, format user agent and check total size
std::vector<string> uacomments;
BOOST_FOREACH(string cmt, mapMultiArgs["-uacomment"])

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 8, 2015

Member

Alternative: error out if SanitizeString(x) != x, instead of silently dropping characters

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 10, 2015

Author Member

Made it to return error~~, but I am thinking about using warnings instead for the whole uacomment thing.~~

@laanwj laanwj added the P2P label Sep 8, 2015
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-uacommentFix branch 2 times, most recently Sep 9, 2015
@MarcoFalke
Copy link
Member Author

MarcoFalke commented Sep 9, 2015

Force pushed changes requested in the comments.

* SanitizeString() can be requested to be more strict
* Throw error when SanitizeString() changes uacomments
* Fix tests
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-uacommentFix branch to 1c1b1b3 Sep 16, 2015
@laanwj laanwj merged commit 1c1b1b3 into bitcoin:master Sep 22, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Sep 22, 2015
1c1b1b3 [uacomment] Sanitize per BIP-0014 (MarcoFalke)
@laanwj
Copy link
Member

laanwj commented Sep 22, 2015

Tested ACK

@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-uacommentFix branch Sep 22, 2015
zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018
Implement uacomment config parameter

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.