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

bindings: close after reading module struct #2792

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Sep 10, 2015

Do not let the module struct to be deallocated by uv_dclose before
reading data from it.

Found it while investigating: #2791 (comment)

cc @nodejs/collaborators

Do not let the module struct to be deallocated by `uv_dclose` before
reading data from it.
@evanlucas
Copy link
Contributor

LGTM

char errmsg[1024];
snprintf(errmsg,
sizeof(errmsg),
"Module version mismatch. Expected %d, got %d.",
NODE_MODULE_VERSION, mp->nm_version);
uv_dlclose(&lib);
Copy link
Member

Choose a reason for hiding this comment

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

mp->nm_version is a pointer to memory from the shared object? Can you add a comment explaining that? It's not very obvious from just looking at the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, I'll add the comment.

@ChALkeR ChALkeR added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 10, 2015
@mscdex
Copy link
Contributor

mscdex commented Sep 10, 2015

uv_dclose should be uv_dlclose in the commit message.

@yosuke-furukawa
Copy link
Member

LGTM2

indutny added a commit that referenced this pull request Sep 10, 2015
Do not let the module struct to be deallocated by `uv_dlclose`
before reading data from it.

PR-URL: #2792
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
@indutny
Copy link
Member Author

indutny commented Sep 10, 2015

Landed in a6d674d, thank you!

@indutny
Copy link
Member Author

indutny commented Sep 10, 2015

@rvagg I suggest to backport this to 4.0

@indutny indutny closed this Sep 10, 2015
@indutny indutny deleted the fix/gh-2791 branch September 10, 2015 19:26
indutny added a commit that referenced this pull request Sep 11, 2015
Do not let the module struct to be deallocated by `uv_dlclose`
before reading data from it.

PR-URL: #2792
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
indutny added a commit that referenced this pull request Sep 12, 2015
Do not let the module struct to be deallocated by `uv_dlclose`
before reading data from it.

PR-URL: #2792
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
@rvagg rvagg mentioned this pull request Sep 12, 2015
@Fishrock123 Fishrock123 mentioned this pull request Sep 13, 2015
7 tasks
@rvagg rvagg mentioned this pull request Sep 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants