Skip to content

gui: patch icu - #11232

Open
LucasYuki wants to merge 2 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:bazel-icu-patch
Open

gui: patch icu#11232
LucasYuki wants to merge 2 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:bazel-icu-patch

Conversation

@LucasYuki

Copy link
Copy Markdown
Contributor

Summary

Fix done using Claude.

QFileDialog's built-in file listing (QFileSystemModel) sorts entries with a locale-aware compare, which constructs a QCollator and loads ICU locale data. The Bazel Central Registry's icu module patches u_getDataDirectory() to locate that data via rules_cc::cc::runfiles::Runfiles::Rlocation(), but Runfiles::Create() returns nullptr whenever no runfiles tree is reachable, and the overlay dereferences it unconditionally. A binary produced by bazel/install.sh has its .runfiles directory stripped after unpacking, so the very first locale-aware compare anywhere in the process — e.g. QFileDialog::getOpenFileName populating its file list — segfaults.

This is the same defect class fixed in #11203 for HelpWidget::sortItems(); that PR's own "Scope" section flagged the file-dialog path as still open: "a file dialog's QFileSystemModel... would still reach the same unchecked pointer."

Rather than patch each call site that happens to trigger a collated sort, this adds a single_version_override on the icu module (pulled in transitively via qt-bazel) that null-checks Runfiles::Create()'s result before calling ->Rlocation() on it, falling through to ICU's existing "" default instead of crashing. This closes the bug for every locale-aware sort in the GUI (file dialogs, sortable timing tables, etc.), not just HelpWidget.

Every icu version currently in the BCR carries the same unchecked dereference (76.1.bcr.4, 78.2.bcr.2, ...), so bumping the version is not a workaround; only patching putil.cpp fixes it. The real fix belongs upstream in the BCR's icu module — this override should be dropped once that lands.

Type of Change

  • Bug fix

Impact

No behavior change for anyone whose binary still has a reachable runfiles tree (e.g. running via bazel run / bazel-bin directly) — Runfiles::Rlocation() still resolves and path is still set exactly as before. For an installed binary (no runfiles tree), ICU's data directory now silently falls back to "" instead of segfaulting; ICU already handles that case gracefully elsewhere in dataDirectoryInitFn(). Fixes the crash reported in #11217 (opening the "Open DB" file dialog) and the equivalent one in OpenROAD-flow-scripts#4454 (DRC Viewer's "Load" file chooser).

Verification

  • I have verified that the local build succeeds (./etc/Build.sh).
  • I have run the relevant tests and they pass.
  • My code follows the repository's formatting guidelines.
  • I have included tests to prevent regressions.
  • I have signed my commits (DCO).

Verified directly at the Bazel module level rather than a full ./etc/Build.sh run:

  • bazelisk mod graph resolves icu@76.1.bcr.3 with the override applied.
  • bazelisk fetch on the real @icu external repo, then inspected the materialized source on disk — the null-check is present after the BCR's own overlay patches are applied on top of it.
  • bazelisk build @@icu+//icu4c/source/common:platform (the target compiling the patched putil.cpp) builds cleanly.
  • Confirmed by counter-test: moving the same override into qt_bazel_prebuilts's own MODULE.bazel (a non-root module here) causes Bazel to silently ignore it and refetch the unpatched, vulnerable source — confirming the override must live in whichever module is root (openroad here, and orfs for the ORFS-side build), not in qt-bazel itself.

No regression test added: this is a Bazel dependency-patch fix with no C++ code path in this repo to exercise, and there's no existing harness for asserting installed-binary runfiles-absent behavior (same reasoning as #11203).

Related Issues

Fixes #11217
Related: OpenROAD-flow-scripts#4454

Signed-off-by: LucasYuki <lucasyuki@yahoo.com.br>
@LucasYuki LucasYuki self-assigned this Aug 25, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a patch to null-check Bazel runfiles in ICU to prevent segmentation faults when binaries are copied out of the bazel-bin directory. However, the patch introduces a critical Use-After-Free (UAF) bug because path is assigned to the c_str() of a local std::string (dat_path) which is destroyed when the function returns. To resolve this, the lifetime of dat_path should be extended by making it static.

Signed-off-by: LucasYuki <lucasyuki@yahoo.com.br>
@LucasYuki
LucasYuki marked this pull request as ready for review August 28, 2026 14:20
@LucasYuki
LucasYuki requested review from a team as code owners August 28, 2026 14:20
@LucasYuki
LucasYuki requested a review from maliberty August 28, 2026 14:20
@LucasYuki

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up commit (gui: disable warning: Could not create collator) to this PR.

Once the icu Runfiles null-check stops the segfault, an installed binary still can't reach ICU's locale data (no runfiles tree, so Runfiles::Rlocation() never resolves). That means QCollator legitimately fails to construct, and Qt logs Could not create collator: 4 (U_FILE_ACCESS_ERROR) every time something sorts through it — e.g. opening a file dialog.

I checked Qt's own qcollator_icu.cpp: when construction fails, QCollator::compare() falls back to a plain ordinal string compare instead of crashing or misbehaving:

if (d->collator) {
    return ucol_strcoll(d->collator, ...);
}
return QtPrivate::compareStrings(s1, s2, d->caseSensitivity);

So this is cosmetic noise, not a functional regression — file listings and sortable tables still sort correctly, just without locale-aware collation (irrelevant for ASCII filenames).

Suppressed it in the existing Qt-message interceptor (message_handler() in gui.cpp), using the same #if NDEBUG-gated pattern already there for the offscreen-platform warning, so it stays visible in debug builds if the underlying ICU data path ever needs debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gui: crash with bazel build when opening db

1 participant