fmt has broken the build again #1978
Conversation
WalkthroughThe pull request replaces two instances of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/gui/gui.cc (2)
416-427: Critical: Potential null pointer dereference (2 instances).Both uses of
std::localtimeat lines 421 and 425 can returnnullptr, which would crash the application when dereferenced. This affects the "Copy to clipboard" feature in the About dialog.Apply this diff to add null safety:
if (version.failed()) { ImGui::EndDisabled(); ImGui::TextUnformatted(_("No version information.\n\nProbably built from source.")); } else { + auto* tm = std::localtime(&version.timestamp); + if (!tm) { + ImGui::TextUnformatted(_("Error: Failed to convert timestamp.")); + ImGui::EndTabItem(); + return changed; + } if (ImGui::Button(_("Copy to clipboard"))) { if (version.buildId.has_value()) { clip::set_text( fmt::format("Version: {}\nBuild: {}\nChangeset: {}\nDate & time: {:%Y-%m-%d %H:%M:%S}", version.version, version.buildId.value(), version.changeset, - *std::localtime(&version.timestamp))); + *tm)); } else { clip::set_text(fmt::format("Version: {}\nChangeset: {}\nDate & time: {:%Y-%m-%d %H:%M:%S}", version.version, version.changeset, - *std::localtime(&version.timestamp))); + *tm)); } }
437-439: Critical: Potential null pointer dereference.Line 437 dereferences the result of
std::localtimewithout checking fornullptr, which can crash the application when displaying the timestamp in the About dialog.Apply this diff to add null safety:
ImGui::SameLine(); if (ImGui::SmallButton(version.changeset.c_str())) { openUrl(fmt::format("https://github.com/grumpycoders/pcsx-redux/commit/{}", version.changeset)); } - std::tm tm = *std::localtime(&version.timestamp); + auto* tmPtr = std::localtime(&version.timestamp); + if (!tmPtr) { + ImGui::TextUnformatted(_("Date & time: Error converting timestamp")); + ImGui::EndTabItem(); + return changed; + } + std::tm tm = *tmPtr; std::string timestamp = fmt::format("{:%Y-%m-%d %H:%M:%S}", tm); ImGui::Text(_("Date & time: %s"), timestamp.c_str());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/gui/gui.cc(2 hunks)src/main/main.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: build
| fmt::print( | ||
| "{{\n \"version\": \"{}\",\n \"changeset\": \"{}\",\n \"timestamp\": \"{}\",\n \"timestampDecoded\": " | ||
| "\"{:%Y-%m-%d %H:%M:%S}\"\n}}\n", | ||
| version.version, version.changeset, version.timestamp, fmt::localtime(version.timestamp)); | ||
| version.version, version.changeset, version.timestamp, *std::localtime(&version.timestamp)); | ||
| return 0; |
There was a problem hiding this comment.
Critical: Potential null pointer dereference.
std::localtime can return nullptr if the timestamp is invalid or a system error occurs. Dereferencing without a null check will cause undefined behavior and likely crash the application.
Consider adding a null check or using a safer alternative:
+ auto* tm = std::localtime(&version.timestamp);
+ if (!tm) {
+ fmt::print("Failed to convert timestamp\n");
+ return 1;
+ }
fmt::print(
"{{\n \"version\": \"{}\",\n \"changeset\": \"{}\",\n \"timestamp\": \"{}\",\n \"timestampDecoded\": "
"\"{:%Y-%m-%d %H:%M:%S}\"\n}}\n",
- version.version, version.changeset, version.timestamp, *std::localtime(&version.timestamp));
+ version.version, version.changeset, version.timestamp, *tm);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt::print( | |
| "{{\n \"version\": \"{}\",\n \"changeset\": \"{}\",\n \"timestamp\": \"{}\",\n \"timestampDecoded\": " | |
| "\"{:%Y-%m-%d %H:%M:%S}\"\n}}\n", | |
| version.version, version.changeset, version.timestamp, fmt::localtime(version.timestamp)); | |
| version.version, version.changeset, version.timestamp, *std::localtime(&version.timestamp)); | |
| return 0; | |
| auto* tm = std::localtime(&version.timestamp); | |
| if (!tm) { | |
| fmt::print("Failed to convert timestamp\n"); | |
| return 1; | |
| } | |
| fmt::print( | |
| "{{\n \"version\": \"{}\",\n \"changeset\": \"{}\",\n \"timestamp\": \"{}\",\n \"timestampDecoded\": " | |
| "\"{:%Y-%m-%d %H:%M:%S}\"\n}}\n", | |
| version.version, version.changeset, version.timestamp, *tm); | |
| return 0; |
🤖 Prompt for AI Agents
In src/main/main.cc around lines 218-222, the code dereferences the pointer
returned by std::localtime without checking for nullptr which can crash if
std::localtime fails; update the code to call std::localtime (or the thread-safe
localtime_r/localtime_s variant) into a std::tm pointer, check that the pointer
is not null before using it, and if it is null produce a safe fallback (e.g.,
log/print an "invalid timestamp" string or the raw integer timestamp) so the
fmt::print call never dereferences a null pointer.
2 participants
fmt::localtime was removed in favor of std::localtime, so I had to change it to that to be able to build the emulator