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

RFE: open files with O_CLOEXEC #704

Open
ensc opened this issue Jul 23, 2023 · 6 comments
Open

RFE: open files with O_CLOEXEC #704

ensc opened this issue Jul 23, 2023 · 6 comments

Comments

@ensc
Copy link

ensc commented Jul 23, 2023

Files should be opened so that they do not leak their fd to child processes.

Lot of POSIX systems (GNU, BSD) support the "e" flag in fopen(). E.g fopen(file, "re").

It would be nice when leptonica applies this flag where possible.

@DanBloomberg
Copy link
Owner

I hadn't heard of this arg. Running man on fopen(3) doesn't show the 'e' flag.
From gnu documentation

    https://www.gnu.org/software/libc/manual/html_node/Opening-Streams.html

it is a GNU extension. The GNU documentation says that it is equivalent to setting FD_CLOEXEC.
However, Fedora's documentation

    https://docs.fedoraproject.org/en-US/defensive-coding/tasks/Tasks-Descriptors/

says this can cause race conditions, and instead one should use the socket-levl O_CLOEXEC which is supported on some systems.

I don't know the safest way to proceed.

@ensc
Copy link
Author

ensc commented Jul 24, 2023

it is a GNU extension

most relevant POSIX systems support it in the meantime:

afair there is some discussion to make it officially a part of POSIX

Fedora's documentation ... says this can cause race conditions

Fedora doc says that the traditional way (open() + fcntl()) is racy and open(..., O_CLOEXEC) shall be used instead.

The "e" flag is implemented with O_CLOEXEC; it does not use a manual fcntl()

@DanBloomberg
Copy link
Owner

Is it correct that systems that don't support 'e' will always ignore it?

If so, would you suggest I add the 'e' arg to the fopen() call in fopenReadStream() in utils2.c?
Is it sufficient to substitute "fbe" for "fb" in those calls?

What about the arg to fopenWriteStream()? Should I append 'e' to the modestring in that call?

@ensc
Copy link
Author

ensc commented Jul 24, 2023

Is it correct that systems that don't support 'e' will always ignore it?

I do not know this but other multi-platform projects are setting this flag too.

When you want to be sure, add a ./configure check which sets

#define FOPEN_E_FLAG "e"

and then

    fopen(..., "rb" FOPEN_E_FLAG);

Btw, I just read that Windows uses "N" for this purpose (https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen)

What about the arg to fopenWriteStream()? Should I append 'e' to the modestring in that call?

yes; it seems that modestring must be modified at runtime.

@DanBloomberg
Copy link
Owner

Thanks for the suggestion. To avoid breaking on some platforms, I think it might be best to give this issue some time to get standardized.

Anything we do has to be set up with both autotools and cmake, and work on all the usual platforms.

The Windows documentation says that their 'n' is not ANSI and shouldn't be used on other platforms. Also, it dioes not seem to have the same effect as 'e', but I couldn't really tell.

Question: why not compile in the modification of modestring when opening for write?

@ensc
Copy link
Author

ensc commented Jul 25, 2023

The Windows documentation says that their 'n'

uppercase 'N': "Specifies that the file isn't inherited by child processes"; sounds similar, but I really do not have any clue about Windows...

Question: why not compile in the modification of modestring when opening for write?

I mean, for readStream() it is simple because you can write "rb" FOPEN_E_FLAG directly which is evaluated at compile time.

But writeStream() takes the mode as argument so that you have to manipulate it at runtime.

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

No branches or pull requests

2 participants