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

1. 
   practicalswift commented at 8:45 am on May 2, 2018: contributor

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

   freopen(…) frees m_fileout.

2. logging: Fix potential use-after-free in LogPrintStr(...) 76f344de6d
3. fanquake added the label Utils/log/libs on May 2, 2018
4. 
   laanwj commented at 9:18 am on May 2, 2018: member

   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.
5. logging: remove unused return value from LogPrintStr

   `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

   0bd4cd398b
6. 
   practicalswift commented at 9:26 am on May 2, 2018: contributor

   @laanwj Nice cleanup! Added that commit to this PR.
7. 
   laanwj commented at 9:53 am on May 2, 2018: member

   utACK https://github.com/bitcoin/bitcoin/pull/13148/commits/0bd4cd398b19c2ef90e2f39b40a60b9fa915d2b2
8. 
   promag commented at 10:55 am on May 2, 2018: member

   utACK 0bd4cd3.
9. laanwj merged this on May 3, 2018
10. laanwj closed this on May 3, 2018

    ---

11. laanwj referenced this in commit b62b437acd on May 3, 2018
12. 
    ajtowns commented at 1:05 pm on May 3, 2018: member

    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:

     0     if (m_reopen_file) {
     1          m_reopen_file = false;
     2          FILE* new_fileout = fopen(m_file_path, "a");
     3          if (!new_fileout) {
     4               // release m_file_mutex first
     5               LogPrintf("Failed to reopen log file %s\n", m_file_path);
     6          } else {
     7               fflush(m_fileout);
     8               fclose(m_fileout);
     9               m_fileout = new_fileout;
    10          }
    11     }
    

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

13. luke-jr referenced this in commit 448935e011 on Jul 7, 2018
14. laanwj referenced this in commit bb0277819a on Aug 31, 2018
15. jasonbcox referenced this in commit 1d91c62d14 on Sep 13, 2019
16. jasonbcox referenced this in commit 3bdbe48216 on Sep 13, 2019
17. jonspock referenced this in commit cb6ac65be1 on Dec 22, 2019
18. jonspock referenced this in commit 130286a16d on Dec 22, 2019
19. proteanx referenced this in commit 268df15a73 on Dec 23, 2019
20. proteanx referenced this in commit 910826634c on Dec 23, 2019
21. random-zebra referenced this in commit a10317adc1 on Feb 21, 2021
22. practicalswift deleted the branch on Apr 10, 2021
23. UdjinM6 referenced this in commit ed721566da on May 21, 2021
24. UdjinM6 referenced this in commit a03d649a48 on May 25, 2021
25. TheArbitrator referenced this in commit ffe806b91d on Jun 7, 2021
26. Munkybooty referenced this in commit a58bf3ca3f on Jun 30, 2021
27. Munkybooty referenced this in commit 561093f425 on Jul 2, 2021
28. Munkybooty referenced this in commit 2be36d4db5 on Jul 2, 2021
29. gades referenced this in commit aa0558cb65 on Apr 21, 2022
30. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift laanwj promag ajtowns

Labels
Utils/log/libs