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

Fix fonts on NixOS and any other XDG-compliant distro with unusual directory structure #31

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

easrng
Copy link

@easrng easrng commented Jun 3, 2024

NixOS doesn't have /usr/share/fonts, so Ladybird was only loading the default bitmap font. This fixes that by replacing hardcoded paths (/usr/share/fonts, /usr/local/share/fonts, $HOME/.local/share/fonts) with their XDG base directory equivalents (with fallbacks included so nothing changes on systems where $XDG_DATA_HOME or $XDG_DATA_DIRS are unset). It also silences the warnings when nonexistent directories are returned from StandardPaths.cpp::font_directories(), since that is to be expected with the updated code that checks for more possible directories.

Userland/Libraries/LibCore/StandardPaths.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibCore/StandardPaths.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibGfx/Font/FontDatabase.cpp Outdated Show resolved Hide resolved
@ADKaster
Copy link
Member

ADKaster commented Jun 3, 2024

Whoops, looks like the code I gave you, or the edits you made (either or) are now causing a use after free https://github.com/LadybirdWebBrowser/ladybird/actions/runs/9357541272/job/25757496121?pr=31#step:14:47

Do feel free to resolve resolved comments yourself btw, the only people who can do that on github are the author and people with write access.

@easrng
Copy link
Author

easrng commented Jun 3, 2024

the tests pass locally so idk how to fix this with no debug feedback... trying a rebuild with CC=clang CXX=clang++

@ADKaster
Copy link
Member

ADKaster commented Jun 3, 2024

I haven't quite updated it yet, but there is a RunningTests.md documentation file. Basically, poke CMake to -DENABLE_UNDEFINED_SANITIZER=ON and -DENABLE_ADDRESS_SANITIZER=ON and set the associated OPTIONS environment variables to reproduce the CI failure

@fgaz
Copy link
Contributor

fgaz commented Jun 5, 2024

fyi this was temporarily fixed downstream by hardcoding the nixos font path in NixOS/nixpkgs#315048

@ADKaster
Copy link
Member

ADKaster commented Jun 6, 2024

The bug was that you had

Core::Environment::get("XDG_DATA_DIRS"_string).value_or("/usr/local/share:/usr/share"_string);

However, Core::Environment::get() returns Optional<StringView>. So when XDG_DATA_DIRS was not defined, we use-after-free'd the temporary string in the value_or branch.

The fix is straightforward: Don't use temporary strings, use stringview literals

Core::Environment::get("XDG_DATA_DIRS"sv).value_or("/usr/local/share:/usr/share"sv);

I'll push this tiny fix to your branch and merge it soon

@ADKaster ADKaster merged commit 43b8f82 into LadybirdBrowser:master Jun 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants