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

rtdl: Return null when non-elf file is dlopened #901

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

Qwinci
Copy link
Contributor

@Qwinci Qwinci commented Aug 18, 2023

Right now it just fails at an assert in _fetchFromFile, it should just return null according to the man page.

On success, dlopen() and dlmopen() return a non-NULL handle for
the loaded object. On error (file could not be found, was not
readable, had the wrong format, or caused errors during loading),
these functions return NULL.

Copy link
Member

@no92 no92 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Dennisbonke Dennisbonke left a comment

Choose a reason for hiding this comment

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

LGTM

@qookei
Copy link
Member

qookei commented Aug 18, 2023

Why not make _fetchFromFile return some value to indicate failure instead of duplicating the checks

@Qwinci
Copy link
Contributor Author

Qwinci commented Aug 18, 2023

Why not make _fetchFromFile return some value to indicate failure instead of duplicating the checks

Actually thats a better idea, Ill do that

@Qwinci Qwinci force-pushed the rtdl-nonelf branch 2 times, most recently from 35e2436 to 47ef7fe Compare August 18, 2023 12:45
@Qwinci Qwinci marked this pull request as draft August 18, 2023 12:50
@Qwinci Qwinci marked this pull request as ready for review August 18, 2023 12:54
@Geertiebear
Copy link
Member

Wouldn't it be a lot cleaner to use frg::expected in this case?

@Qwinci
Copy link
Contributor Author

Qwinci commented Aug 21, 2023

Wouldn't it be a lot cleaner to use frg::expected in this case?

Instead of the bools? Not sure how would that make it cleaner though.

Copy link
Member

@Geertiebear Geertiebear left a comment

Choose a reason for hiding this comment

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

Instead of lastError being a global accross files it could stay private to main.cpp. The functions can then return a frg::expected<void, LinkerError> where LinkerError is some enum. This way we keep the error handling out of linker.cpp.

@Qwinci
Copy link
Contributor Author

Qwinci commented Feb 16, 2024

Fixed and also added more checks to the elf to make sure that it also has the correct machine and bitness.

Copy link
Member

@qookei qookei left a comment

Choose a reason for hiding this comment

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

Looks good apart from the comments.

options/rtdl/generic/main.cpp Outdated Show resolved Hide resolved
options/rtdl/generic/linker.hpp Show resolved Hide resolved
options/rtdl/generic/linker.cpp Outdated Show resolved Hide resolved
Copy link
Member

@no92 no92 left a comment

Choose a reason for hiding this comment

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

LGTM

@mintsuki mintsuki merged commit bfd94f8 into managarm:master Feb 16, 2024
30 checks passed
@Qwinci Qwinci deleted the rtdl-nonelf branch February 16, 2024 18:46
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.

6 participants