Details

Reviewers

deadalnix
freetrader
kyuupichan
awemany

Group Reviewers

Restricted Project

Commits

rSTAGING1db4ca8f354a: Check for user agent string length and trim it if necessary
rABC1db4ca8f354a: Check for user agent string length and trim it if necessary

Summary

Fix issues raised at https://reviews.bitcoinabc.org/T55

Test Plan

create a unit test to check userAgent() return value

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

sickpig created this revision.Jun 28 2017, 16:22

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 28 2017, 16:22

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

Harbormaster completed remote builds in B440: Diff 715.Jun 28 2017, 19:22

freetrader added inline comments.Jun 28 2017, 21:31

src/test/net_tests.cpp
191 ↗	(On Diff #715)	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).

freetrader added inline comments.Jun 28 2017, 21:46

src/net.cpp
2820 ↗	(On Diff #715)	typo: length

sickpig marked an inline comment as done.Jun 29 2017, 07:32

sickpig added inline comments.

src/test/net_tests.cpp
191 ↗	(On Diff #715)	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 ↗	(On Diff #715)	seems like Core did the same on commit `52922456`

freetrader added inline comments.Jun 29 2017, 09:01

src/util.cpp
413 ↗	(On Diff #715)	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

freetrader added inline comments.Jun 29 2017, 10:24

src/util.cpp
413 ↗	(On Diff #715)	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

Harbormaster completed remote builds in B442: Diff 717.Jun 29 2017, 14:03

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

deadalnix accepted this revision.Jun 29 2017, 21:20

This revision is now accepted and ready to land.Jun 29 2017, 21:20

Closed by commit rABC1db4ca8f354a: Check for user agent string length and trim it if necessary (authored by sickpig, committed by sickpig). · Explain WhyJun 30 2017, 06:09

This revision was automatically updated to reflect the committed changes.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 718

src/net.cpp

src/test/net_tests.cpp

src/util.h

src/util.cpp

Check for user agent string length and trim it if necessaryClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 718

src/net.cpp

src/test/net_tests.cpp

src/util.h

src/util.cpp

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

Revision Contents
Changeset List