Skip to content

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

Open
zenbooster opened this issue Jul 28, 2022 · 11 comments
Open

fcntl_fcntl_impl 1024 bytes limitation #95380

zenbooster opened this issue Jul 28, 2022 · 11 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@zenbooster
Copy link

zenbooster commented Jul 28, 2022

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

@zenbooster zenbooster added the type-feature A feature request or enhancement label Jul 28, 2022
@gst
Copy link
Contributor

gst commented Jul 28, 2022

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();

?

@gst
Copy link
Contributor

gst commented Jul 28, 2022

I would be very happy to contribute and make the fix/MR by the way.

@tiran
Copy link
Member

tiran commented Jul 29, 2022

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.

PyObject *ret = PyBytes_FromStringAndSize(NULL, len);
if (ret == NULL) {
    return NULL;
}
char *buf = PyBytes_AS_STRING(ret);
memcpy(buf, str, len);
...
return ret;

@zenbooster
Copy link
Author

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.

@gst
Copy link
Contributor

gst commented Jul 29, 2022

I like this option better.

I did it like that :)

@gst
Copy link
Contributor

gst commented Jul 29, 2022

@zenbooster

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 ?

@arhadthedev
Copy link
Member

not sure what is eventually best to do.

Take everything of fcntl_fcntl_impl into gh-95429 and leave fcntl_ioctl_impl in gh-95439?

@gst
Copy link
Contributor

gst commented Jul 29, 2022

Take everything of fcntl_fcntl_impl into #95429 and leave fcntl_ioctl_impl in #95439?

seems good for me.

so I can leave #95429 as is ?

@arhadthedev
Copy link
Member

so I can leave #95429 as is ?

Right. BTW I forgot to preliminary check that the only difference between the PRs in fcntl_fcntl_impl is in names.

zenbooster added a commit to zenbooster/cpython that referenced this issue Aug 1, 2022
@iritkatriel iritkatriel added the extension-modules C modules in the Modules dir label Nov 27, 2023
@serhiy-storchaka
Copy link
Member

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.

@serhiy-storchaka
Copy link
Member

#132907 is an alternative PR which keeps using a 1024 bytes buffer for small arguments. It also does the same for ioctl() and adds tests for large buffers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

6 participants