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.

Sign up for GitHub

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

Jump to bottom

fix fd leaks in fdopendir #29

Merged
merged 1 commit into from
Sep 4, 2020
Merged

fix fd leaks in fdopendir #29

merged 1 commit into from
Sep 4, 2020

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Aug 27, 2020

fdopendir is supposed to take control of its dirfd parameter upon success.
An unrelated edge case in error handling was also fixed to not leak memory and a fd.

See evanw/esbuild#348 for context.

@kzc
Copy link
Contributor Author

kzc commented Aug 27, 2020 •
edited
Loading

Somewhat on topic... I have no idea how macports works, but I see the port status of go 1.15 is red for 10.9 and prior OSX versions. I was able to get Go and any program built with it to work on OSX 10.9.5 using the instructions in evanw/esbuild#81 (comment).

@ryandesign
Copy link
Contributor

I was able to get Go and any program built with it to work on OSX 10.9.5 using the instructions in evanw/esbuild#81 (comment).

Thanks, I've added a link to your comment to https://trac.macports.org/ticket/58935#comment:33 which is where we are tracking the go failure on 10.9 and earlier.

@kzc
Copy link
Contributor Author

kzc commented Aug 27, 2020

go can be built on 10.9 using legacy support and DYLD_INSERT_LIBRARIES

I updated my comment in the esbuild thread to reflect that the go toolchain was not built - the official golang release was untarred and used as-is.

michaelld
michaelld requested changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@michaelld michaelld left a comment

Choose a reason for hiding this comment

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

A couple things to tweak. Otherwise, I think the changes do what you want & make sense to bring this function into conformity with standards-expected behavior.

src/fdopendir.c Outdated
if (dirfd == AT_FDCWD) {
dir = opendir (".");
/* dirfd can be closed only upon success */
if (dir && dirfd != -1) PROTECT_ERRNO(close(dirfd));
Copy link
Contributor

Choose a reason for hiding this comment

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

We knows here that dirfd != -1 (since it equals AT_FDCWD, which is -2 according to sys/fcntl.h), so that part can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/fdopendir.c Show resolved Hide resolved
michaelld
michaelld approved these changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@michaelld michaelld left a comment

Choose a reason for hiding this comment

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

LGTM now; thx!

src/fdopendir.c Show resolved Hide resolved
@kzc
Copy link
Contributor Author

kzc commented Sep 3, 2020

@kencu Any concerns with this PR? It allows the official go 1.15 release and any program created with it to work correctly on OSX 10.9.5 in file handle constrained environments.

@kencu
Copy link
Contributor

kencu commented Sep 3, 2020

Sorry, I didn't see this before.

I'll have to look it over and try it out on 10.4 to whereever it tops out to see what happens. In the meantime, you have made these changes on your own system I presume so you have that working for you.

@kencu
Copy link
Contributor

kencu commented Sep 4, 2020

looks reasonable, thank you for the time you spent improving our legacy library. We have hopes that this will prove ever more useful for those of us with older Apple systems that are still useful hardware-wise, if abandoned software-wise by upstream.

@kencu kencu merged commit 3c9d3c2 into macports:master Sep 4, 2020
@kzc
Copy link
Contributor Author

kzc commented Sep 4, 2020

Thanks to everyone in macports who make this great software.

Getting go to support the C bindings on systems where upstream has dumped support is tricky, though. If you have skills in this area, much appreciate your help. Specifically, making go link in accessory libraries like legacy-support is a supreme PITA.

Would install_name_tool help in this regard?

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

@michaelld michaelld michaelld approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kzc @ryandesign @kencu @michaelld