Skip to content

Remove strtol in torcontrol #23538

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Nov 25, 2021
Merged

Remove strtol in torcontrol #23538

laanwj merged 1 commit into from
Nov 25, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 17, 2021 •
edited
Loading

The sequence of octal chars is fully validated before calling strtol, so it can be replaced by a simple loop. This removes the last "locale depended" strtol call. Also, removes some unused includes.

@katesalazar
Copy link
Contributor

katesalazar commented Nov 17, 2021 •
edited
Loading

Concept ACK.

Cppreference.com: std::from_chars_result from_chars(...): locale independence refers to code initially proposed and later removed completely.

flack
flack reviewed Nov 17, 2021
View reviewed changes
@fanquake
Copy link
Member

fanquake commented Nov 18, 2021

https://github.com/bitcoin/bitcoin/pull/23538/checks?check_run_id=4242747766

torcontrol.cpp:279:49: runtime error: implicit conversion from type 'unsigned char' of value 255 (8-bit, unsigned) to type 'char' changed the value to -1 (8-bit, signed)
    #0 0x557c2b741c35 in ParseTorReplyMapping(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) src/torcontrol.cpp:279:49
    #1 0x557c2ad31efb in torcontrol_tests::CheckParseTorReplyMapping(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >) src/test/torcontrol_tests.cpp:61:16
    #2 0x557c2ad2efd8 in torcontrol_tests::util_ParseTorReplyMapping::test_method() src/test/torcontrol_tests.cpp:150:5
    #3 0x557c2ad2ceea in torcontrol_tests::util_ParseTorReplyMapping_invoker() src/test/torcontrol_tests.cpp:73:1
    #4 0x557c2a46a89f in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:117:11
    #5 0x7fad31b494c1  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x274c1)
    #6 0x7fad31b4f410 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2d410)
    #7 0x7fad31b4f900 in boost::execution_monitor::execute(boost::function<int ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2d900)
    #8 0x7fad31b4f9bb in boost::execution_monitor::vexecute(boost::function<void ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2d9bb)
    #9 0x7fad31b73876 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x51876)
    #10 0x7fad31b925a6  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x705a6)
    #11 0x7fad31b92bf3  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x70bf3)
    #12 0x7fad31b92bf3  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x70bf3)
    #13 0x7fad31b5a79a in boost::unit_test::framework::run(unsigned long, bool) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x3879a)
    #14 0x7fad31b6c6f9 in boost::unit_test::unit_test_main(bool (*)(), int, char**) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x4a6f9)
    #15 0x557c2a39d83d in main /usr/include/boost/test/unit_test.hpp:64:12
    #16 0x7fad311bffcf  (/lib/x86_64-linux-gnu/libc.so.6+0x2dfcf)
    #17 0x7fad311c007c in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2e07c)
    #18 0x557c2a2ec974 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x10d3974)

SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change torcontrol.cpp:279:49 in 
make[3]: *** [Makefile:19175: test/torcontrol_tests.cpp.test] Error 1

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 18, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22953 (refactor: introduce single-separator split helper (boost::split replacement) by theStack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Nov 18, 2021
Merged
@laanwj
Copy link
Member

laanwj commented Nov 18, 2021

Concept ACK.
I wondered how this ended up in the code (I definitely didn't remember writing it), but looks like the strtol was introduced in 49a199b

@laanwj
Copy link
Member

laanwj commented Nov 18, 2021 •
edited
Loading

honestly, I kind of wonder why this isn't implemented with a little

num *= 8;
num += val - '0';

kind of loop.
Wrapping it in a std::string then passing it to a parsing function seems overkill, especially if no validation nor special whitespace handling is needed, and it already iterates over every character.

@maflcko maflcko force-pushed the 2111-torcontrol branch 2 times, most recently from fa1d030 to fa9fe9f Compare November 18, 2021 16:53
@maflcko
Copy link
Member Author

maflcko commented Nov 18, 2021

Wrapping it in a std::string then passing it to a parsing function seems overkill, especially if no validation nor special whitespace handling is needed, and it already iterates over every character.

The validation pass needs to be a different loop than the calculation loop, so I think a one-line function call is fine. If speed is a concern, ToIntegral could be changed to use std::string_view instead.

Though, changed to a while loop for now.

@maflcko maflcko changed the title Replace strtol with ToIntegral in torcontrol Remove strtol in torcontrol Nov 18, 2021
@laanwj
Copy link
Member

laanwj commented Nov 24, 2021 •
edited
Loading

Code review and tested ACK fa186eb

If speed is a concern, ToIntegral could be changed to use std::string_view instead.

I don't think speed is a concern, the whole construction just looked fragile and bolted on to me, I think it's much clearer what the code does and what the intent is this way.

@laanwj
Copy link
Member

laanwj commented Nov 25, 2021 •
edited
Loading

It looks like this code is sufficiently tested in util_ParseTorReplyMapping.

I've done some manual testing, too, in this way:

$ src/bitcoind -regtest -listenonion=1 -listen=1 -debug=tor
⋮ 
2021-11-25T10:34:45Z tor: Connected to Tor version 0.4.6.7
2021-11-25T10:34:45Z tor: Supported authentication method: SAFECOOKIE
2021-11-25T10:34:45Z tor: Using SAFECOOKIE authentication, reading cookie authentication from /tmp/💜
2021-11-25T10:34:45Z tor: Authentication cookie /tmp/💜 could not be opened (check permissions)
2021-11-25T10:34:56Z tor: End of stream

$ nc -l -C -p 9051
PROTOCOLINFO 1
250-PROTOCOLINFO 1    
250-AUTH METHODS=SAFECOOKIE COOKIEFILE="/tmp/\360\237\222\234"
250-VERSION Tor="0.4.6.7"
250 OK

@laanwj laanwj merged commit 791dd1f into bitcoin:master Nov 25, 2021
@maflcko maflcko deleted the 2111-torcontrol branch November 25, 2021 11:06
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2021
@laanwj
Merge bitcoin#23538: Remove strtol in torcontrol
c4a7c81
@bitcoin bitcoin locked and limited conversation to collaborators Nov 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@flack flack flack left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @katesalazar @fanquake @DrahtBot @laanwj @flack