refactor: Change occurences of c_str() used with size() to data() #17280

laanwj · 2019-10-28T12:51:44Z

Using data() better communicates the intent here.

Also, depending on how c_str() is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case). Apparently this is no longer an issue with C++11.

Using `data()` better communicates the intent here. Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents.

fjahr · 2019-10-28T14:25:34Z

Code review ACK f3b51eb

practicalswift · 2019-10-28T14:53:43Z

Concept ACK

What about linting this to make sure this is a one-time fix-up?

Suggested lint command:

$ git grep -E '([a-zA-Z0-9_]+).c_str\(\)+[^a-zA-Z0-9_]+\1\.size\(\)' -- "*.cpp" "*.h"
src/bitcoin-cli.cpp:        char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);
src/crypto/hkdf_sha256_32.cpp:    CHMAC_SHA256((const unsigned char*)salt.c_str(), salt.size()).Write(ikm, ikmlen).Finalize(m_prk);
src/fs.cpp:    int size = MultiByteToWideChar(CP_ACP, 0, mb_string.c_str(), mb_string.size(), nullptr, 0);
src/fs.cpp:    MultiByteToWideChar(CP_ACP, 0, mb_string.c_str(), mb_string.size(), &*utf16_string.begin(), size);
src/httprpc.cpp:        CHMAC_SHA256(reinterpret_cast<const unsigned char*>(strSalt.c_str()), strSalt.size()).Write(reinterpret_cast<const unsigned char*>(strPass.c_str()), strPass.size()).Finalize(out);
src/util/strencodings.cpp:    return EncodeBase64((const unsigned char*)str.c_str(), str.size());
src/util/strencodings.cpp:    return EncodeBase32((const unsigned char*)str.c_str(), str.size());
src/wallet/crypter.cpp:    di.Write((const unsigned char*)strKeyData.c_str(), strKeyData.size());

laanwj · 2019-10-28T15:19:56Z

What about linting this to make sure this is a one-time fix-up?

Maybe—It could catch some of them, I guess. But only if they're on the same line?
(which is the case for all of these, admittedly)

practicalswift · 2019-10-28T15:34:22Z

@laanwj Correct, multi-line search with git grep is not possible AFAIK, so there could be false negatives.

ryanofsky

Code review ACK f3b51eb. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain \0 and were probably safe before, but much better to avoid the possibility of bugs like this.

maflcko · 2019-10-28T19:37:26Z

We are using C++11 and according to https://en.cppreference.com/w/cpp/string/basic_string/c_str this is safe.

c_str() and data() perform the same function. | (since C++11)

I have removed the "bug" label and added the "Refactoring" label.

Concept ACK, though.

practicalswift · 2019-10-29T11:20:17Z

ACK f3b51eb -- diff looks correct, data() more idiomatic

… to data() f3b51eb Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan) Pull request description: Using `data()` better communicates the intent here. ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](#17281 (comment)). ACKs for top commit: fjahr: Code review ACK f3b51eb practicalswift: ACK f3b51eb -- diff looks correct, `data()` more idiomatic ryanofsky: Code review ACK f3b51eb. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this. Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8

… size() to data() f3b51eb Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan) Pull request description: Using `data()` better communicates the intent here. ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](bitcoin#17281 (comment)). ACKs for top commit: fjahr: Code review ACK f3b51eb practicalswift: ACK f3b51eb -- diff looks correct, `data()` more idiomatic ryanofsky: Code review ACK f3b51eb. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this. Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8

…e() to data() Summary: Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan) Pull request description: Using `data()` better communicates the intent here. ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](bitcoin/bitcoin#17281 (comment)). --- Backport of Core [[bitcoin/bitcoin#17280 | PR17280]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7387

… size() to data() f3b51eb Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan) Pull request description: Using `data()` better communicates the intent here. ~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](bitcoin#17281 (comment)). ACKs for top commit: fjahr: Code review ACK f3b51eb practicalswift: ACK f3b51eb -- diff looks correct, `data()` more idiomatic ryanofsky: Code review ACK f3b51eb. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this. Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8

… data()

Merge bitcoin#19660, bitcoin#19373, bitcoin#19841, bitcoin#13862, bitcoin#13866, bitcoin#17280, bitcoin#17682 and partial bitcoin#19326, bitcoin#14978: Auxiliary Backports

Fix occurences of c_str() used with size() to data()

f3b51eb

Using `data()` better communicates the intent here. Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents.

laanwj added the Bug label Oct 28, 2019

ryanofsky approved these changes Oct 28, 2019

View reviewed changes

maflcko added Refactoring and removed Bug labels Oct 28, 2019

laanwj changed the title ~~Fix occurences of c_str() used with size() to data()~~ refactor: Change occurences of c_str() used with size() to data() Oct 30, 2019

laanwj merged commit f3b51eb into bitcoin:master Oct 30, 2019

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

kwvg pushed a commit to kwvg/dash that referenced this pull request May 19, 2021

Merge bitcoin#17280: Change occurences of c_str() used with size() to…

2c13748

… data()

kwvg added a commit to kwvg/dash that referenced this pull request May 19, 2021

Merge bitcoin#17280: Change occurences of c_str() used with size() to…

d9489c7

… data()

kwvg mentioned this pull request May 19, 2021

Merge #19660, #19373, #19841, #13862, #13866, #17280, #17682 and partial #19326, #14978: Auxiliary Backports dashpay/dash#4164

Merged

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#17280: Change occurences of c_str() used with size() to…

bc77b43

… data()

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#17280: Change occurences of c_str() used with size() to…

61bfe7b

… data()

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#17280: Change occurences of c_str() used with size() to…

6590f56

… data()

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#17280: Change occurences of c_str() used with size() to…

6a73b70

… data()

str4d mentioned this pull request Dec 6, 2021

Backport changes to src/fs.{cpp,h} zcash/zcash#5406

Open

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Change occurences of c_str() used with size() to data() #17280

refactor: Change occurences of c_str() used with size() to data() #17280

Uh oh!

laanwj commented Oct 28, 2019 •

edited

Loading

Uh oh!

fjahr commented Oct 28, 2019

Uh oh!

practicalswift commented Oct 28, 2019 •

edited

Loading

Uh oh!

laanwj commented Oct 28, 2019 •

edited

Loading

Uh oh!

practicalswift commented Oct 28, 2019 •

edited

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

maflcko commented Oct 28, 2019

Uh oh!

practicalswift commented Oct 29, 2019

Uh oh!

Uh oh!

refactor: Change occurences of c_str() used with size() to data() #17280

refactor: Change occurences of c_str() used with size() to data() #17280

Uh oh!

Conversation

laanwj commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Oct 28, 2019

Uh oh!

practicalswift commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 28, 2019

Uh oh!

practicalswift commented Oct 29, 2019

Uh oh!

Uh oh!

laanwj commented Oct 28, 2019 •

edited

Loading

practicalswift commented Oct 28, 2019 •

edited

Loading

laanwj commented Oct 28, 2019 •

edited

Loading

practicalswift commented Oct 28, 2019 •

edited

Loading