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

stacktrace: Remove dangerous value access #29

Merged
merged 1 commit into from
Sep 3, 2023

Conversation

Bojun-Seo
Copy link
Collaborator

dlip.dli_fname could be NULL.
So add null check before use this value.

@honggyukim
Copy link
Owner

Thanks. I'll have a look later.

@Bojun-Seo
Copy link
Collaborator Author

Bojun-Seo commented Mar 28, 2023

dladdr function could be failed. There could be only three cases. Based on following man page.
https://man7.org/linux/man-pages/man3/dladdr.3.html
Followings are the way to print for each cases.
Consider symbol name as A, symbol offset as B, shared object name as C and shared object offset as D

Case 1: Success
A +B (C +D)
Case 2: Failed to get symbol but succeed to get shared object
(C +D)
Case 3: Failed to get symbol as well as shared object
unknown

Copy link
Owner

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. I've just made a few comments. Thanks for your help!

src/stacktrace.cc Outdated Show resolved Hide resolved
Comment on lines 113 to 116
if (dlip.dli_sname != NULL && dlip.dli_saddr != NULL) {
symbol = abi::__cxa_demangle(dlip.dli_sname, 0, 0, &status);
if (status != 0)
symbol = strdup(dlip.dli_sname);
Copy link
Owner

Choose a reason for hiding this comment

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

I think this change is not needed because we already added return value check of dladdr above.

Copy link
Collaborator Author

@Bojun-Seo Bojun-Seo Apr 11, 2023

Choose a reason for hiding this comment

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

There could be a case that dladdr could return non-zero value but dli_sname and dli_saddr is NULL. You can find that case on commit message "Case 2: Failed to get symbol but succeed to get shared object".

dladdr function could be failed.
There could be only three cases.
Followings are the way to print for each cases.
Consider symbol name as A, symbol offset as B, shared object name as C
and shared object offset as D

Case 1: Success
  A +B (C +D)
Case 2: Failed to get symbol but succeed to get shared object
  (C +D)
Case 3: Failed to get symbol as well as shared object
  unknown

Signed-off-by: Bojun Seo <bojun.seo.0@gmail.com>
Copy link
Owner

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

I've slightly modified your commit changing NULL to nullptr, then LGTM now.

Thanks for your contribution!

@honggyukim honggyukim merged commit 3968671 into honggyukim:main Sep 3, 2023
@Bojun-Seo Bojun-Seo deleted the bug branch December 18, 2023 02:25
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.

2 participants