log: Check that the timestamp string is non-empty to avoid undefined behavior #27317
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
src/logging.cpp
Outdated
There was a problem hiding this comment.
This should never happen, so instead of adding dead code, what about simply combining the two if conditions?
There was a problem hiding this comment.
My thinking was that if it never happens, then this PR is unnecessary. If it could happen, then I'd rather have an explicit message rather than output that entirely lacks timestamps, which someone may not notice is bizarre.
Happy to change it, though, if people prefer your approach.
There was a problem hiding this comment.
Assuming it can happen, maybe wrap the message in, say, square brackets ("[Error obtaining current timestamp]") to be clearer in the log.
There was a problem hiding this comment.
I think I've already come around to the view that it's so unlikely as to be not worth the additional LoC, so I'll switch to the simpler version.
Thanks!
There was a problem hiding this comment.
I've seen this happening to computers before, but never when bitcoind was running. Usually that happens when time synchronization goes wrong for whatever reason.
There was a problem hiding this comment.
If there are steps to reproduce this, it would be good to write a detection for it at Bitcoin Core init time and refuse to start the program (if this isn't already done).
The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds since epoch to a formatted date time. In the unlikely case that happens, `strStamped.pop_back()` would be undefined behavior.
e68230a to
73f4eb5
Compare
| const auto now_seconds{std::chrono::time_point_cast<std::chrono::seconds>(now)}; | ||
| strStamped = FormatISO8601DateTime(TicksSinceEpoch<std::chrono::seconds>(now_seconds)); | ||
| if (m_log_time_micros) { | ||
| if (m_log_time_micros && !strStamped.empty()) { |
There was a problem hiding this comment.
Could wrap this in an Assume() to both make it clear to the reader that it's not supposed to happen and easier to catch any (very unexpected) bugs without crashing non-development builds?
| if (m_log_time_micros && !strStamped.empty()) { | |
| if (m_log_time_micros && Assume(!strStamped.empty())) { |
|
lgtm ACK 73f4eb5 |
…y to avoid undefined behavior 73f4eb5 Check that the Timestamp String is valid (John Moffett) Pull request description: Follow-up to bitcoin#27233 The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior. ACKs for top commit: MarcoFalke: lgtm ACK 73f4eb5 stickies-v: ACK 73f4eb5 Tree-SHA512: 089ed639c193deb98870a8385039b31b4baed821ea907937bfc6f65a5b0981bbf8284b2afec81b2d0a922e2340716b48cf55349640eb6b8c311ef7af25abc361
Follow-up to #27233
The current
FormatISO8601DateTimefunction will return an empty string if it encounters an error when converting theint64_tseconds-since-epoch to a formatted date time. In the unlikely case that happens, herestrStamped.pop_back()would be undefined behavior.