Remove last occurrence of potentially insecure function sprintf #1777

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2012_09_eliminate_sprintf changing 1 files +8 −10

laanwj commented at 6:29 AM on September 3, 2012: member

%d can return up to 11 characters (12 with \0). Move away from fixed buffer completely and use our own safe function strprintf.

I'm reasonably sure that this cannot be exploited due to earlier checks, but it does make me feel a tad uneasy.

Remove last occurrence of potentially insecure function sprintf.

%d can return up to 11 characters. Move away from fixed buffer completely and
use our own safe function strprintf.

9c80909452

in src/net.cpp:None in 0e6a129b47 outdated

1062 | @@ -1065,23 +1063,23 @@ void ThreadMapPort2(void* parg)
1063 |  #ifndef UPNPDISCOVER_SUCCESS
1064 |          /* miniupnpc 1.5 */
1065 |          r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype,
1066 | -                            port, port, lanaddr, strDesc.c_str(), "TCP", 0);
1067 | +                            port..c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", 0);

Diapolo commented at 8:26 AM on September 3, 2012:

It seems you have a "." too much here.

laanwj commented at 8:29 AM on September 3, 2012:

Yep. Good catch.

in src/net.cpp:None in 0e6a129b47 outdated
```
1019 | @@ -1020,9 +1020,7 @@ void ThreadMapPort2(void* parg)
1020 |  {
1021 |      printf("ThreadMapPort started\n");
1022 |  
1023 | -    char port[6];
1024 | -    sprintf(port, "%d", GetListenPort());
1025 | -
1026 | +    std::string port = strprintf("%d", GetListenPort());
```
Diapolo commented at 8:28 AM on September 3, 2012:

I removed my former comment, but want to make another suggestion. Why not use?

#include <boost/lexical_cast.hpp> std::string port = boost::lexical_cast<std::string>(GetListenPort());

http://www.boost.org/doc/libs/1_51_0/doc/html/boost_lexical_cast.html

laanwj commented at 2:45 PM on September 3, 2012:

That's possible, but I don't see the advantage. We don't use lexical_cast anywhere right now, and we have the strprintf function already.

Diapolo commented at 2:47 PM on September 3, 2012:

You are right, I was just asking, if a own strprintf makes sense in that context, as I (but that is a pure feeling) would consider boost functions more safe than own string-handling code. I'm fine with leaving that pull as is anyway :).
Diapolo commented at 2:24 PM on September 3, 2012: none

Is it possible to remove the use of sprintf here, too? https://github.com/bitcoin/bitcoin/blob/master/src/uint256.h#L291
laanwj commented at 2:41 PM on September 3, 2012: member

I thought I removed that sprintf months ago. At least I rewrote a similar function in util.cpp. That function should use the existing HexStr function.

Edit: I see the reason now. uint256.h cannot include util.h, as util.h includes uint256.h, So uint256 cannot simply use HexStr unless we move that to a third .h file...
jgarzik referenced this in commit 2fd6f067db on Sep 4, 2012
jgarzik merged this on Sep 4, 2012
jgarzik closed this on Sep 4, 2012
laanwj deleted the branch on Apr 9, 2014
owlhooter referenced this in commit f37a64208c on Oct 10, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

laanwj

Diapolo