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

Add public Py_fopen() and Py_fclose() functions, and remove private _Py_fopen_obj() function #51

Closed
10 of 12 tasks
vstinner opened this issue Dec 16, 2024 · 8 comments
Closed
10 of 12 tasks

Comments

@vstinner
Copy link

vstinner commented Dec 16, 2024

Hi,

Python 3.13 removed the private function _Py_wfopen(). A possible replacement is the private _Py_fopen_obj() function. Instead, I propose adding a clean public tested and documented Py_fopen() function, with a companion Py_fclose() function.

API:

FILE* Py_fopen(PyObject *path, const char *mode)
int Py_fclose(FILE *file)

Py_fopen() is similar to the fopen() function, but the path argument is a Python object (instead of char*), and an exception is set on error.

On Windows, files opened by Py_fopen() in the Python DLL must be closed by the Python DLL to use the same C runtime version. Otherwise, calling fclose() directly can cause undefined behavior. So I also propose adding a Py_close() function to avoid this problem.

Finally, I propose to remove the untested and undocumented _Py_fopen_obj() function, to promote the usage of the public tested and documented Py_fopen() function.

Py_fopen() and Py_fclose() are needed by the long list of C API functions which expect a FILE* argument.

Add Py_fopen() and Py_close():

Remove _Py_fopen_obj():

@zooba
Copy link

zooba commented Dec 16, 2024

I think the new functions are good, and essential, so we should add them.

I'd prefer to keep the old name around for another version or two, even if it's only an alias for the new name, so that users have a bit more overlap in which to change the name (i.e. if they care about 3.13 and 3.14, they can keep using the old name for now).

@vstinner
Copy link
Author

For the backward compatibility, I plan to add these functions to pythoncapi-compat, so they will be usable on Python 3.6+.

@encukou
Copy link
Collaborator

encukou commented Dec 16, 2024

Ideally everyone would use Python file objects instead of FILE*, but this works for backwards compatibility.

I plan to add these functions to pythoncapi-compat

How will Py_fclose work there?

@vstinner
Copy link
Author

vstinner commented Dec 16, 2024

How will Py_fclose work there?

I guess that it will just call fclose(). I don't know if it would work as expected or not on Windows.

@zooba
Copy link

zooba commented Dec 16, 2024

Ideally everyone would use Python file objects instead of FILE*, but this works for backwards compatibility.

Yeah, we went through the "ideally" on the bug already, and there are just too many public/"stable" APIs taking FILE* to ignore.

At least being able to keep the FILE entirely on the Python side makes it reliable. I wouldn't be opposed to creating our own typedef for FILE* to reinforce that, but it probably breaks existing users.

How will Py_fclose work there?

I guess that it will just call fclose(). I don't think if it would work as expected or not on Windows

I imagine it would break for the user who reported the issue in the first place, since they were using static linking.1 But there really isn't a way around that as far as I can tell. And also no way to detect it other than trying to do it and hopefully failing (as opposed to crashing or corrupting data).

Footnotes

  1. To clarify and save searching: if you statically link the C runtime, but dynamically link to CPython, Python is using a different C runtime from the rest of your app.

@serhiy-storchaka
Copy link

On Windows, files opened by Py_fopen() in the Python DLL must be closed by the Python DLL to use the same C runtime version. Otherwise, calling fclose() directly can cause undefined behavior. So I also propose adding a Py_close() function to avoid this problem.

Could you please give a reference? This looks like a bad news. Some of the highlevel C API can close the input file. If Py_fopen() is only compatible with Py_fopen(), then we cannot use fdopen(), freopen(), _fsopen(), fmemopen(), etc.

@zooba
Copy link

zooba commented Jan 1, 2025

Unfortunately, I'm not aware of any direct reference. The team behind the runtimes consider mixing runtime implementations out of their scope, so they don't document the things that may happen.

To derive the problem from first principles, you have to recognise that distinct DLLs have their own global variables, that static linking essentially treats the "host" process/DLL as the global variable scope, that the debug UCRT is a different DLL from the standard one (and both can be loaded simultaneously), that open files in the UCRT are stored in a global variable1, and that the fopen implementation allocates objects in this array (trust me? or start tracing from stdio/fopen.cpp until you reach lowio/osfinfo.cpp).

Thus, multiple instances of the UCRT will have multiple instances of the array and are allocating file objects with the same index but different addresses. The FILE * struct contains a reference to the correct handle (rather than the descriptor/index), but is going to use an incorrect definition for the layout of what's at that handle.

If Py_fopen() is only compatible with Py_fopen(), then we cannot use fdopen(), freopen(), _fsopen(), fmemopen(), etc.

You can if you compile with normal settings. If you statically link the C runtime, or insist on using a debug CRT with a release CPython (using the release CRT), then you may well encounter issues. But when all the libraries use the same runtime, which is the usual case, there's no problem.

Footnotes

  1. You can also find this file in a Windows SDK install, but this random copy of it seems accurate enough.

@vstinner
Copy link
Author

vstinner commented Jan 6, 2025

The Py_fopen() and Py_fclose() APIs are accepted by the C API Working Group.

Since removing _Py_fopen_obj() didn't reach a majority vote, let's keep it for now.

@vstinner vstinner closed this as completed Jan 6, 2025
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

4 participants