-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
fcntl_fcntl_impl 1024 bytes limitation #95380
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
Comments
something like that : 09:21 $ git diff
diff --git a/Modules/fcntlmodule.c b/Modules/fcntlmodule.c
index 9a8ec8dc98..3b7b9d8f1d 100644
--- a/Modules/fcntlmodule.c
+++ b/Modules/fcntlmodule.c
@@ -54,7 +54,6 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg)
int ret;
char *str;
Py_ssize_t len;
- char buf[1024];
int async_err = 0;
if (PySys_Audit("fcntl.fcntl", "iiO", fd, code, arg ? arg : Py_None) < 0) {
@@ -65,9 +64,10 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg)
int parse_result;
if (PyArg_Parse(arg, "s#", &str, &len)) {
- if ((size_t)len > sizeof buf) {
- PyErr_SetString(PyExc_ValueError,
- "fcntl string arg too long");
+ char* buf = malloc((size_t)len);
+ PyObject* buf_ret;
+ if (buf == NULL) {
+ PyErr_NoMemory();
return NULL;
}
memcpy(buf, str, len);
@@ -77,9 +77,12 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg)
Py_END_ALLOW_THREADS
} while (ret == -1 && errno == EINTR && !(async_err = PyErr_CheckSignals()));
if (ret < 0) {
+ free(buf);
return !async_err ? PyErr_SetFromErrno(PyExc_OSError) : NULL;
}
- return PyBytes_FromStringAndSize(buf, len);
+ buf_ret = PyBytes_FromStringAndSize(buf, len);
+ free(buf);
+ return buf_ret;
}
PyErr_Clear(); ? |
I would be very happy to contribute and make the fix/MR by the way. |
There is even a better way that avoids the extra memory allocation. You can create a new, empty bytes object and directly access its buffer. You are allowed to do this if the object is new and has not left the scope of the current C function.
|
I like this option better. Also, in the fcntl_ioctl_impl function, I removed adding zero to the end of the parameter for ioctl. Removed - works. |
I did it like that :) |
seems like we overlapped :) not sure what is eventually best to do. if you are ok with that : can you make a diff to my branch and I'll merge/cherry-pick the commit into it ? |
Right. BTW I forgot to preliminary check that the only difference between the PRs in |
python#95380 (comment) fcntl_ioctl_impl: add Py_DECREF.
There may be an issue with this. Currently, when you pass too small buffer, you have still a chance to get reasonable (truncated) result or OSError. When you directly pass a small buffer to fcntl() or ioctl(), you have larger chance to get a segfault or memory corruption. It is a bug in your program, but severity of it will be changed, as well as a chance for recovery. I think that it is safer to use that technique only for large buffer, and continue to use the temporary buffer for small arguments. |
#132907 is an alternative PR which keeps using a 1024 bytes buffer for small arguments. It also does the same for |
char buf[1024];
Why not allocate the buffer dynamically to remove this limitation? I am passing a structure to the driver that contains, among other things, the name of the target device of size PATH_MAX, and I do not fit into 1024 bytes. Everything works in the client in C, but not in python with your module.
Linked PRs
The text was updated successfully, but these errors were encountered: