Replace remaining sprintf with snprintf #9867

laanwj · 2017-02-26T20:14:02Z

Use of sprintf is seen as a red flag as many of its uses are insecure. OpenBSD warns about it while compiling, and some modern platforms, e.g. cloudlibc from cloudabi don't even provide it anymore.

Although our uses of these functions are secure, it can't hurt to replace them anyway. There are only 4 occurences of which 3 in the tests.

gmaxwell

utACK.

paveljanik · 2017-02-26T20:48:53Z

ACK d9b6b87

JeremyRubin · 2017-02-26T21:43:03Z

src/uint256.cpp

@@ -22,7 +22,7 @@ std::string base_blob<BITS>::GetHex() const
 {
    char psz[sizeof(data) * 2 + 1];
    for (unsigned int i = 0; i < sizeof(data); i++)
-        sprintf(psz + i * 2, "%02x", data[sizeof(data) - i - 1]);
+        snprintf(psz + i * 2, 3, "%02x", data[sizeof(data) - i - 1]);


This is a little bit sketchy. Could it not safely be 2 here, not 3?

NUL term needs a 3rd byte.

For others http://www.cplusplus.com/reference/cstdio/snprintf/

A terminating null character is automatically appended after the content written.

Hence, 2 hex characters + 1 null character.

It's correct, and it's also why snprintf isn't an especially safe function call. (replaces one class of issue with another more subtle one)

It's correct, and it's also why snprintf isn't an especially safe function call. (replaces one class of issue with another more subtle one)

Indeed, that's why we never use it. The go-to function we have for this is "strprintf". However, it's kind of high overhead for formatting two hex characters. Especially as this function is called a lot.

I'll just replace this with use of HexDigit instead.

TheBlueMatt · 2017-02-26T21:43:08Z

Concept ACK for general sanity, though I'm pretty sure there is no reason we should go out of our way to support libcs that are deliberately broken.

laanwj · 2017-02-27T06:26:14Z

Concept ACK for general sanity, though I'm pretty sure there is no reason we should go out of our way to support libcs that are deliberately broken.

Somewhat tend to agree, though there is something to be said for deprecating unsafe functions that have caused so much pain since the 80's. They do the same with system calls, trying to keep the API (anda attack surface) as minimalist as possible.

laanwj · 2017-02-27T06:49:20Z

Force-pushed into two commits:

uint256: replace use of sprintf with per-digit hex conversion
test: Replace remaining sprintf with snprintf

dcousens · 2017-02-27T06:51:01Z

src/uint256.cpp

+    static const char *hex_chars = "0123456789abcdef";
+    std::string retval;
+    retval.reserve(sizeof(data) * 2);
+    for (unsigned int i = 0; i < sizeof(data); i++) {


why not size_t?

Meh. For loop counters it's generally enough to use a 32-bit type. Especially if you know you're only going to count to 64, ever.

dcousens · 2017-02-27T06:53:57Z

src/uint256.cpp

+    for (unsigned int i = 0; i < sizeof(data); i++) {
+        uint8_t value = data[sizeof(data) - i - 1];
+        retval.push_back(hex_chars[value >> 4]);
+        retval.push_back(hex_chars[value & 0xf]);


edit: fail

But incorrect.

nit: maybe emplace rather than push (potentially saves an initialization)
@dcousens can you not delete your feedback? I had to dig in my emails to see that I wasn't repeating your feedback.

Do you really think there's anything to be gained by emplacing a char? I'd say something has to be really wrong for that to make a difference.

Anyhow I have a better solution based on reverse_iterator coming, so no need to discuss this further.

I'm pretty sure that the copy constructor and move constructor for a char are the same, so emplace wouldn't make a difference.

@JeremyRubin I made a woefully incorrect suggestion in regards to the bitmasking.

retval.push_back(hex_chars[value & 0xf0]); retval.push_back(hex_chars[value & 0x0f]);

@dcousens no worries we all have our dumb moments.

laanwj · 2017-02-27T08:34:18Z

src/uint256.cpp

@@ -20,10 +20,15 @@ base_blob<BITS>::base_blob(const std::vector<unsigned char>& vch)
 template <unsigned int BITS>
 std::string base_blob<BITS>::GetHex() const


Possibly we could even use HexStr here with a reverse_iterator, though I don't know how to do that for a C-style array. All the variations I can think of are much worse than just replicating this functionality here.
Edit: found it out, it's simple!

#include "stdio.h" #include <iterator> int main() { char a[2] = {'a', 'b'}; auto start = std::reverse_iterator<char *>(&a[2]); auto end = std::reverse_iterator<char *>(&a[0]); while (start != end) printf("%c", *(start++)); }

Outputs:

ba

Yes, thanks, I'm aware, see the new version of this patch.

ha -- and here I was thinking you coded it after I commented and I was like "wow, @laanwj is a god amongst men in code response times" ;)

Yes, I implemented it in minus 3 minutes! Truly godlike :-)

Instead of calling sprintf for every byte, format the hex bytes ourselves by help of HexStr and a reverse_iterator.

Use of `sprintf` is seen as a red flag as many of its uses are insecure. OpenBSD warns about it while compiling, and some modern platforms, e.g. [cloudlibc from cloudabi](https://github.com/NuxiNL/cloudlibc) don't even provide it anymore. Although our uses of these functions are secure, it can't hurt to replace them anyway. There are only 3 occurences left, all in the tests.

laanwj · 2017-02-27T09:04:34Z

There you go, a one-liner now.

JeremyRubin · 2017-02-27T09:19:34Z

utACK 19cafc6

practicalswift · 2017-02-27T22:17:30Z

utACK 19cafc6

sipa · 2017-02-27T23:23:59Z

utACK

gmaxwell

re-utACK.

kallewoof · 2017-02-28T00:10:10Z

utACK 19cafc6

jonasschnelli

utACK 19cafc6

19cafc6 test: Replace remaining sprintf with snprintf (Wladimir J. van der Laan) 0a17714 uint256: replace sprintf with HexStr and reverse-iterator (Wladimir J. van der Laan) Tree-SHA512: 2ba1dd4d25e1cbfff4d67b2f483448aa7c34ab5c799cddd48ba5826e5fa6df425abe35e244aaf4c52db9fccfb4d2a25a14bb4597bf9d1fce95991f270da6bb26

Additional dbwrapper tests Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7992 - bitcoin/bitcoin#9867 - Only the commit affecting dbwrapper tests - bitcoin/bitcoin#9610 - Only the change affecting dbwrapper tests - bitcoin/bitcoin#10844

19cafc6 test: Replace remaining sprintf with snprintf (Wladimir J. van der Laan) 0a17714 uint256: replace sprintf with HexStr and reverse-iterator (Wladimir J. van der Laan) Tree-SHA512: 2ba1dd4d25e1cbfff4d67b2f483448aa7c34ab5c799cddd48ba5826e5fa6df425abe35e244aaf4c52db9fccfb4d2a25a14bb4597bf9d1fce95991f270da6bb26

Lint fixes Fixes most lints currently reported by `test/lint/lint-all.sh`. Includes changes cherry-picked from the following upstream PRs: - bitcoin/bitcoin#8700 - bitcoin/bitcoin#8840 - bitcoin/bitcoin#9867 - We backported the second commit in #3146 - bitcoin/bitcoin#10771 - bitcoin/bitcoin#11394 - bitcoin/bitcoin#11649 - bitcoin/bitcoin#17329 - bitcoin/bitcoin#19258

bc3d3fa Add testcase to simulate bitcoin schema in leveldb (MapleLaker) 4f15155 Verify DBWrapper iterators are taking snapshots (Matt Corallo) 36b407e test: Replace remaining sprintf with snprintf (Wladimir J. van der Laan) e8cb38a Add test for dbwrapper iterators with same-prefix keys. (Matt Corallo) 4a4ddb8 test: Add more thorough test for dbwrapper iterators I made a silly mistake in a database wrapper where keys were sorted by char instead of uint8_t. As x86 char is signed the sorting for the block index database was messed up, resulting in a segfault due to missing records. (Wladimir J. van der Laan) 109446f chain: Add assertion in case of missing records in index db (Wladimir J. van der Laan) Pull request description: Back ported the following PRs from upstream (adaptations were needed because we aren't obfuscating the db): * bitcoin#7992. * bitcoin#9867. * bitcoin#11422. * bitcoin#17206 ACKs for top commit: random-zebra: Nice! ACK bc3d3fa Tree-SHA512: ebd811d7ee0d970247ceec2624e524df7611274f36ef7c0252712ebf247828ee091dcc6872585271fac3d7846127a997a856d7cd72eb20425e06213207ff1c7a

laanwj added the Refactoring label Feb 26, 2017

gmaxwell approved these changes Feb 26, 2017

View reviewed changes

JeremyRubin reviewed Feb 26, 2017

View reviewed changes

laanwj force-pushed the 2017_02_snprintf branch from d9b6b87 to 30dc477 Compare February 27, 2017 06:49

dcousens reviewed Feb 27, 2017

View reviewed changes

dcousens approved these changes Feb 27, 2017

View reviewed changes

dcousens reviewed Feb 27, 2017

View reviewed changes

laanwj commented Feb 27, 2017

View reviewed changes

laanwj added 2 commits February 27, 2017 10:02

uint256: replace sprintf with HexStr and reverse-iterator

0a17714

Instead of calling sprintf for every byte, format the hex bytes ourselves by help of HexStr and a reverse_iterator.

laanwj force-pushed the 2017_02_snprintf branch from 30dc477 to 19cafc6 Compare February 27, 2017 09:03

dcousens approved these changes Feb 27, 2017

View reviewed changes

gmaxwell approved these changes Feb 27, 2017

View reviewed changes

jonasschnelli approved these changes Feb 28, 2017

View reviewed changes

laanwj merged commit 19cafc6 into bitcoin:master Feb 28, 2017

str4d mentioned this pull request Apr 4, 2018

Additional dbwrapper tests zcash/zcash#3146

Merged

Krekeler added a commit to Krekeler/documentchain that referenced this pull request Jun 14, 2020

Merge bitcoin#9867 and bitcoin#0a17714

6a72789

str4d mentioned this pull request Nov 9, 2020

Lint fixes zcash/zcash#4849

Merged

furszy mentioned this pull request Apr 13, 2021

[Test] Expand dbwrapper unit test coverage PIVX-Project/PIVX#2313

Merged

barton2526 mentioned this pull request Aug 4, 2021

Remove sprintf from the codebase gridcoin-community/Gridcoin-Research#2264

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -20,10 +20,15 @@ base_blob<BITS>::base_blob(const std::vector<unsigned char>& vch)
		template <unsigned int BITS>
		std::string base_blob<BITS>::GetHex() const

Replace remaining sprintf with snprintf #9867

Replace remaining sprintf with snprintf #9867

Uh oh!

Conversation

laanwj commented Feb 26, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Feb 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 26, 2017

Uh oh!

laanwj commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 27, 2017

Uh oh!

JeremyRubin commented Feb 27, 2017

Uh oh!

practicalswift commented Feb 27, 2017

Uh oh!

sipa commented Feb 27, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

dcousens Feb 27, 2017 •

edited

Loading

gmaxwell Feb 27, 2017 •

edited

Loading

laanwj Feb 27, 2017 •

edited

Loading

laanwj commented Feb 27, 2017 •

edited

Loading

dcousens Feb 27, 2017 •

edited

Loading

sipa Feb 27, 2017 •

edited

Loading

laanwj Feb 27, 2017 •

edited

Loading