fcntl_fcntl_impl 1024 bytes limitation

[zenbooster]

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

• gh-95380: Removing the 1024 bytes limit in the fcntl_ioctl_impl functions. #95526
• gh-95380: Remove the 1024 bytes limit in fcntl.fcntl() and fcntl.ioctl() #132907

[gst]

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]

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

[tiran]

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]

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]

I like this option better.

I did it like that :)

[gst]

@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]

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]

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]

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.

[serhiy-storchaka]

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.