Skip to content
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.

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

[Qt] Terminate string *pszExePath after readlink and without using memset #11193

Merged

Conversation

practicalswift
Copy link
Contributor

Terminate string *pszExePath after readlink and before passing to operator <<.

  • ssize_t readlink(const char *pathname, char *buf, size_t bufsiz) does not append a null byte to buf.
  • Operator << expects a null-terminated string.

@practicalswift practicalswift changed the title Terminate string *pszExePath after readlink and before passing to operator << [Qt] Terminate string *pszExePath after readlink and before passing to operator << Aug 29, 2017
@jonasschnelli
Copy link
Contributor

Doesn't the memset to all zeros before the readlink ensures that it will be null byte terminated?

@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 30, 2017

@jonasschnelli Yes, you're correct but the current memset method seems a bit overkill for the task at hand. Instead of prematurely setting 1025 bytes to zero my suggestion is to set only one byte to zero when needed :-)

In addition to being more efficient and less surprising, this has the added benefit of making some static analyzers happy - I've seen at least one static analyzer getting confused by the current "indirect" string termination logic via memset used here.

@laanwj
Copy link
Member

laanwj commented Sep 5, 2017

IMO the PR title should be reworded - we already take care of NUL-terminating, just in a different way. This does not fix a potential bug or edge case as is implied.

the current memset method seems a bit overkill for the task at hand.

True in general, although this is not a performance sensitive place. Not sure this change is worth it.

@practicalswift practicalswift changed the title [Qt] Terminate string *pszExePath after readlink and before passing to operator << [Qt] Terminate string *pszExePath after readlink and without using memset Sep 6, 2017
@practicalswift
Copy link
Contributor Author

PR title and commit message reworded :-)

@laanwj laanwj merged commit 3a4401a into bitcoin:master Oct 2, 2017
@laanwj
Copy link
Member

laanwj commented Oct 2, 2017

utACK 3a4401a
This is obviously correct, but I suggest focusing on actual problems and issues in future PRs instead of "this looks better".

laanwj added a commit that referenced this pull request Oct 2, 2017
…thout using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
@TheBlueMatt
Copy link
Contributor

Postumous utACK, though feels like needless code churn, agree with @laanwj it just creates more review burden/time.

Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 30, 2019
…thout using memset

Summary:
3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d

Backport of Core PR11193
bitcoin/bitcoin#11193

Test Plan:
  make check
  ./bitcoin-qt

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3933
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
…thout using memset

Summary:
3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d

Backport of Core PR11193
bitcoin/bitcoin#11193

Test Plan:
  make check
  ./bitcoin-qt

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3933
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
…thout using memset

Summary:
3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d

Backport of Core PR11193
bitcoin/bitcoin#11193

Test Plan:
  make check
  ./bitcoin-qt

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3933
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
…thout using memset

Summary:
3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d

Backport of Core PR11193
bitcoin/bitcoin#11193

Test Plan:
  make check
  ./bitcoin-qt

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3933
proteanx pushed a commit to devaultcrypto/devault that referenced this pull request Dec 12, 2019
…thout using memset

Summary:
3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d

Backport of Core PR11193
bitcoin/bitcoin#11193

Test Plan:
  make check
  ./bitcoin-qt

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3933
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
… and without using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
… and without using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
… and without using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
… and without using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
… and without using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
… and without using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request Feb 14, 2021
84b4185 [Tests] raise minimum fee in feature_blockindexstats (random-zebra)
39d8a20 [Cleanup] Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt (random-zebra)
613e1da [BUG] Fix sizeof in paymentservertests (random-zebra)
0f3e961 [Cleanup] Remove useless call (random-zebra)
b1ca5e0 Remove unused fsbridge::freopen (practicalswift)
b3a1d84 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)
ecfbcfd [Cleanup] Remove unused function AddInvalidSpendsToMap (random-zebra)
15b018c [Refactor] Use InsecureRand in the unit tests (random-zebra)
81fd84c [Refactoring] Replace risky call to rand() with GetRandInt() (random-zebra)
8ba1978 [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)
b6cd719 Remove unreachable code (g_rpcSignals.PostCommand) (practicalswift)
e4f9ab3 [BUG] Memory leak after new CNode / ConnectNode (random-zebra)
536122b Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
13cad19 fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
aa832d8 [Refactoring] Dereference before/after null check (random-zebra)
4630b7d [Trivial] Pass big parameters by reference, not value (random-zebra)
aa7bc7f [Refactoring] Remove unneeded CScript::IsNormalPaymentScript (random-zebra)
6cf3c8f [Trivial] Unitialized scalar fields and variables (random-zebra)

Pull request description:

  This is a collection of small fixes for the following issues/defects (discovered with the coverity tool):

  - Big parameters passed by value
  - calls to rand() function
  - Dereference before/after null-pointer check
  - Pointer to local variable out of scope
  - Resource leak with call to ConnectNode
  - Non-floating point result - unintended integer division (bitcoin#9288)
  - String not-null-terminated (bitcoin#11193)
  - Structurally dead code (bitcoin#9575)
  - Unchecked boolean return value of functions (GetOp, GetTransaction, TxOutToPublicCoin)
  - Unitialized pointers and scalar fields
  - Illegal memory access (bitcoin#13159)
  - Useless call to pubkey.IsCompressed()
  - Wrong `sizeof` argument (bitcoin#11024 + revert #494)

ACKs for top commit:
  furszy:
    Looking good, ACK 84b4185
  Fuzzbawls:
    ACK 84b4185

Tree-SHA512: 4c920a1858ccde65795c090e4becc47c50c0a78db7ab11935b4d3e2bea3f7f1f8ca3b48ce633d8112673092c35ae7cd05c444ed2b16e283a305c765874e55c1c
@practicalswift practicalswift deleted the null-terminate-after-readlink branch April 10, 2021 19:32
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 10, 2022
… and without using memset

3a4401a [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)

Pull request description:

  Terminate string `*pszExePath` after `readlink` and before passing to operator `<<`.

  * `ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)` does not append a null byte to `buf`.
  * Operator `<<` expects a null-terminated string.

Tree-SHA512: fc18844bb23059fead8db0cb9b4b4ba6188f58e3f19ab4719c2737cc5dd6df23ae7d4804ef2820d39b334204a48ee3de1d202c272bcd156e60761af2fcb9349d
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants