Sanitize uacomment #6647

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:MarcoFalke-2015-uacommentFix changing 4 files +36 −12

MarcoFalke commented at 10:55 AM on September 7, 2015: member
- SanitizeString() can be requested to be more strict
- Actually apply SanitizeString() to uacomments
in src/utilstrencodings.h:None in 63d8457f97 outdated
```
  21 | @@ -22,7 +22,15 @@
  22 |  /** This is needed because the foreach macro can't get over the comma in pair<t1, t2> */
  23 |  #define PAIRTYPE(t1, t2)    std::pair<t1, t2>
  24 |  
  25 | -std::string SanitizeString(const std::string& str);
  26 | +static const int SAFE_CHARS_DEFAULT = 0; //!< Default rule in SanitizeString()
```
jonasschnelli commented at 11:50 AM on September 7, 2015:

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

laanwj commented at 2:32 PM on September 8, 2015:

+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.
in src/utilstrencodings.cpp:None in 63d8457f97 outdated
```
  25 | -    string strResult;
  26 | +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890 .,;_/:?@()", // SAFE_CHARS_DEFAULT
  27 | +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890 .,;_?@" // SAFE_CHARS_UACOMMENT
  28 | +};
  29 | +
  30 | +string SanitizeString(const string& str, int rule)
```
jonasschnelli commented at 11:54 AM on September 7, 2015:

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 commented at 11:55 AM on September 7, 2015: contributor

Concept ACK.

in src/init.cpp:None in 63d8457f97 outdated

1013 | @@ -1014,8 +1014,11 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1014 |  
1015 |      RegisterNodeSignals(GetNodeSignals());
1016 |  
1017 | -    // format user agent, check total size
1018 | -    strSubVersion = FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, mapMultiArgs.count("-uacomment") ? mapMultiArgs["-uacomment"] : std::vector<string>());
1019 | +    // sanitize comments per BIP-0014, format user agent and check total size
1020 | +    std::vector<string> uacomments;
1021 | +    BOOST_FOREACH(string cmt, mapMultiArgs["-uacomment"])

laanwj commented at 2:34 PM on September 8, 2015:

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

MarcoFalke commented at 11:09 PM on September 10, 2015:

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

laanwj added the label P2P on Sep 8, 2015
MarcoFalke force-pushed on Sep 9, 2015
MarcoFalke force-pushed on Sep 9, 2015
MarcoFalke commented at 1:49 PM on September 9, 2015: member

Force pushed changes requested in the comments.

[uacomment] Sanitize per BIP-0014

* SanitizeString() can be requested to be more strict
* Throw error when SanitizeString() changes uacomments
* Fix tests

1c1b1b315f

MarcoFalke force-pushed on Sep 16, 2015
laanwj merged this on Sep 22, 2015
laanwj closed this on Sep 22, 2015
laanwj referenced this in commit a3babc826d on Sep 22, 2015
laanwj commented at 9:37 AM on September 22, 2015: member

Tested ACK
MarcoFalke deleted the branch on Sep 22, 2015
zkbot referenced this in commit f1aeaec471 on Mar 21, 2018
zkbot referenced this in commit c97906e265 on Apr 13, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels