Page MenuHomePhabricator

Check for user agent string length and trim it if necessary
ClosedPublic

Authored by sickpig on Jun 28 2017, 16:22.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 440
Build 440: arc lint + arc unit

Event Timeline

WIP, need to add a unit test to validate userAgent return value

With a test, that'd be great. You can mock parameters in test. See miner_tests.cpp for instance.

src/net.cpp
2808 ↗(On Diff #709)

This guy also need a "\n"

Add cpp test case to check max user agent string len
Format a LogPrints in net.cpp

src/test/net_tests.cpp
191

I'd expect a check that the data used for the truncated uacomment is what you expect.

If the string is too long, the data could be taken from the front, the back, the middle (?).
The test should show clearly what is the expected behavior in this case.

It should also clearly show that a comment of length MAX_SUBVERSION_LENGTH is taken as-is and not modified (unless sanitized).

src/net.cpp
2820

typo: length

sickpig added inline comments.
src/test/net_tests.cpp
191

Ok will add a test to check actual content of a trimmed UA and one for UA with a length equal to MAX_SUBVERSION_LENGTH

src/util.cpp
413

seems like Core did the same on commit 52922456

src/util.cpp
413

As this is only used in tests, would be good to use mapMultiArgs and clear() like Core does, ensuring no multiple entries and maybe also document that this method is intended for use by tests only.

fix a typo
expand log aout sanitize uacomment
add a comment for ForceSetArg

src/util.cpp
413

Forget about using mapMultiArgs, I overlooked it is const, just keep using _mapMultiArgs then.

New function to add multi parameters
Add regex based test to check how userAgent() trim too long subversion string

Changes look good to me.

Tested using make check and to verify that the new test case is executed:

$ src/test/test_bitcoin --log_level=all | grep test_userAgentLength
Entering test case "test_userAgentLength"
Leaving test case "test_userAgentLength"; testing time: 8012mks

*** No errors detected
This revision is now accepted and ready to land.Jun 29 2017, 21:20
This revision was automatically updated to reflect the committed changes.