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

logging: Fix potential use-after-free in LogPrintStr(...) #13148

Merged
merged 2 commits into from
May 3, 2018

Conversation

practicalswift
Copy link
Contributor

Fix potential use-after-free in LogPrintStr(...).

freopen(…) frees m_fileout.

@laanwj
Copy link
Member

laanwj commented May 2, 2018

Feel free to take over https://github.com/laanwj/bitcoin/tree/2018_05_logprintf_remove_return_value, which removes the unused and incorrect return value from LogPrintstr and has this commit rebased on top.

`LogPrintStr` returns the number of characters printed. This number is
doubled if both logging to console and logging to file is enabled. As
the return value is never used, I've opted to remove it instead of try
to fix it.

Credit: @laanwj
@practicalswift
Copy link
Contributor Author

practicalswift commented May 2, 2018

@laanwj Nice cleanup! Added that commit to this PR.

@laanwj
Copy link
Member

laanwj commented May 2, 2018

utACK 0bd4cd3

@promag
Copy link
Contributor

promag commented May 2, 2018

utACK 0bd4cd3.

@laanwj laanwj merged commit 0bd4cd3 into bitcoin:master May 3, 2018
laanwj added a commit that referenced this pull request May 3, 2018
0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6
@ajtowns
Copy link
Contributor

ajtowns commented May 3, 2018

I don't think this patch is really correct -- in the usual failure case where the old file handle is closed, but a new file can't be (re)opened (due to permissions, eg), we'll silently drop the current log message, then start writing any future log messages to the m_msgs_before_open buffer, which will never be cleared. Am I missing something?

If I'm not, seems like better behaviour might be something like:

     if (m_reopen_file) {
          m_reopen_file = false;
          FILE* new_fileout = fopen(m_file_path, "a");
          if (!new_fileout) {
               // release m_file_mutex first
               LogPrintf("Failed to reopen log file %s\n", m_file_path);
          } else {
               fflush(m_fileout);
               fclose(m_fileout);
               m_fileout = new_fileout;
          }
     }

(Would it make sense to do StartShutdown() if logging stops working?)

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jul 7, 2018
laanwj added a commit that referenced this pull request Aug 31, 2018
…trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: #13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 13, 2019
Summary:
0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6

Backport of Core PR13148
bitcoin/bitcoin#13148

Test Plan:
  make check

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

Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3976
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 13, 2019
…trying to re-open (on SIGHUP)

Summary:
75ea00f391b742e435c650aae3e827aad913d552 Remove unused fsbridge::freopen (practicalswift)
cceedbc4bf1056db17e0adf76d0db45b94777671 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin/bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

Backport of Core PR13159
bitcoin/bitcoin#13159

Depends on D3976

Test Plan:
  make check
  ./bitcoind -regtest -daemon
  cd <path>/.bitcoin/regtest
  mv debug.log{,.old}
  touch debug.log; chmod 400 debug.log
  killall -HUP bitcoind
  <patch>/bitcoin-abc/build/src/bitcoin-cli -regtest savemempool
Verify that the new debug.log file is empty and debug.log.old continued to be written to

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

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3979
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 22, 2019
Summary:
0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6

Backport of Core PR13148
bitcoin/bitcoin#13148

Test Plan:
  make check

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

Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3976
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 22, 2019
…trying to re-open (on SIGHUP)

Summary:
75ea00f391b742e435c650aae3e827aad913d552 Remove unused fsbridge::freopen (practicalswift)
cceedbc4bf1056db17e0adf76d0db45b94777671 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin/bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

Backport of Core PR13159
bitcoin/bitcoin#13159

Depends on D3976

Test Plan:
  make check
  ./bitcoind -regtest -daemon
  cd <path>/.bitcoin/regtest
  mv debug.log{,.old}
  touch debug.log; chmod 400 debug.log
  killall -HUP bitcoind
  <patch>/bitcoin-abc/build/src/bitcoin-cli -regtest savemempool
Verify that the new debug.log file is empty and debug.log.old continued to be written to

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

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3979
proteanx pushed a commit to devaultcrypto/devault that referenced this pull request Dec 23, 2019
Summary:
0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6

Backport of Core PR13148
bitcoin/bitcoin#13148

Test Plan:
  make check

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

Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3976
proteanx pushed a commit to devaultcrypto/devault that referenced this pull request Dec 23, 2019
…trying to re-open (on SIGHUP)

Summary:
75ea00f391b742e435c650aae3e827aad913d552 Remove unused fsbridge::freopen (practicalswift)
cceedbc4bf1056db17e0adf76d0db45b94777671 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin/bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

Backport of Core PR13159
bitcoin/bitcoin#13159

Depends on D3976

Test Plan:
  make check
  ./bitcoind -regtest -daemon
  cd <path>/.bitcoin/regtest
  mv debug.log{,.old}
  touch debug.log; chmod 400 debug.log
  killall -HUP bitcoind
  <patch>/bitcoin-abc/build/src/bitcoin-cli -regtest savemempool
Verify that the new debug.log file is empty and debug.log.old continued to be written to

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

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3979
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Feb 21, 2021
e5476df trivial: enable print_to_file also with print_to_console (random-zebra)
bb40861 logging: remove unused return value from LogPrintStr (random-zebra)

Pull request description:

  Bug introduced in b3a1d84: `LogPrintStr` no longer returning the character count, because bitcoin#13148 should have been included in the backport.

ACKs for top commit:
  furszy:
    Code review ACK e5476df
  Fuzzbawls:
    ACK e5476df

Tree-SHA512: 51a2e9942a9106a975d45b7f2106ee2426a04d1ec1dc2f1fa8bafd5065205464f61b07e1da0a20fa6ca3237098f18537415bffa1addd5e2736cc22acbc31dd0d
@practicalswift practicalswift deleted the logprintstr-uaf branch April 10, 2021 19:34
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request May 21, 2021
…tStr(...)

0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request May 25, 2021
…tStr(...)

0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6
TheArbitrator pushed a commit to TheArbitrator/dash that referenced this pull request Jun 7, 2021
…tStr(...)

0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jun 30, 2021
…y when trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

# Conflicts:
#	src/logging.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 2, 2021
…y when trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

# Conflicts:
#	src/logging.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 2, 2021
…y when trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

# Conflicts:
#	src/logging.cpp
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Apr 21, 2022
…tStr(...)

0bd4cd3 logging: remove unused return value from LogPrintStr (practicalswift)
76f344d logging: Fix potential use-after-free in LogPrintStr(...) (practicalswift)

Pull request description:

  Fix potential use-after-free in `LogPrintStr(...)`.

  `freopen(…)` frees `m_fileout`.

Tree-SHA512: ceee1f659c10a21525aa648377afeea0a37016339f5269dea54850ba3b475aa316f4931081655717b65f981598fdc9d79a1e79e55f7084c242eeb7bf372bc4b6
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants