[issue9647] os.confstr() does not handle value changing length between calls

[STINNER Victor]

STINNER Victor  added the comment:

This issue is 9 years old and has patches: it is no newcomer friendly, I remove 
the "easy" keyword.

--
keywords:  -easy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[David Watson]

David Watson added the comment:

Here are the alternative patches to allow more than two calls to
confstr().  One patch set just keeps reallocating the buffer
until it's big enough, while the other makes a limited number of
attempts (in this case 20) before raising RuntimeError.

--
keywords: +patch
Added file: http://bugs.python.org/file37428/confstr-realloc-endless-2.7.diff
Added file: http://bugs.python.org/file37429/confstr-realloc-endless-3.4.diff
Added file: http://bugs.python.org/file37430/confstr-realloc-endless-3.5.diff
Added file: http://bugs.python.org/file37431/confstr-realloc-limited-2.7.diff
Added file: http://bugs.python.org/file37432/confstr-realloc-limited-3.4.diff
Added file: http://bugs.python.org/file37433/confstr-realloc-limited-3.5.diff

___
Python tracker 

___# HG changeset patch
# Parent 1edff7001f589dffaf8de4aebcb65a1f1d7deb5c

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -7936,12 +7936,24 @@ posix_confstr(PyObject *self, PyObject *
 PyObject *result = NULL;
 int name;
 char buffer[256];
+char *recvbuf = buffer;
+char *allocated = NULL;
+size_t buflen = sizeof(buffer);
+size_t len;
 
 if (PyArg_ParseTuple(args, "O&:confstr", conv_confstr_confname, &name)) {
-int len;
-
-errno = 0;
-len = confstr(name, buffer, sizeof(buffer));
+
+/* Reset errno before each call as we may need to check it afterwards */
+while (errno = 0, (len = confstr(name, recvbuf, buflen)) > buflen) {
+recvbuf = PyMem_Realloc(allocated, len);
+if (recvbuf == NULL) {
+PyErr_NoMemory();
+goto finally;
+}
+allocated = recvbuf;
+buflen = len;
+}
+
 if (len == 0) {
 if (errno) {
 posix_error();
@@ -7952,15 +7964,11 @@ posix_confstr(PyObject *self, PyObject *
 }
 }
 else {
-if ((unsigned int)len >= sizeof(buffer)) {
-result = PyString_FromStringAndSize(NULL, len-1);
-if (result != NULL)
-confstr(name, PyString_AS_STRING(result), len);
-}
-else
-result = PyString_FromStringAndSize(buffer, len-1);
-}
-}
+result = PyString_FromStringAndSize(recvbuf, len-1);
+}
+}
+finally:
+PyMem_Free(allocated);
 return result;
 }
 #endif
# HG changeset patch
# Parent aba5f771f5ec9e4004c0375ab4302f246a04bae7

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -9727,33 +9727,38 @@ posix_confstr(PyObject *self, PyObject *
 PyObject *result = NULL;
 int name;
 char buffer[255];
+char *recvbuf = buffer;
+char *allocated = NULL;
+size_t buflen = sizeof(buffer);
 size_t len;
 
 if (!PyArg_ParseTuple(args, "O&:confstr", conv_confstr_confname, &name))
 return NULL;
 
-errno = 0;
-len = confstr(name, buffer, sizeof(buffer));
+/* Reset errno before each call as we may need to check it afterwards */
+while (errno = 0, (len = confstr(name, recvbuf, buflen)) > buflen) {
+recvbuf = PyMem_Realloc(allocated, len);
+if (recvbuf == NULL) {
+PyErr_NoMemory();
+goto finally;
+}
+allocated = recvbuf;
+buflen = len;
+}
+
 if (len == 0) {
 if (errno) {
 posix_error();
-return NULL;
 }
 else {
-Py_RETURN_NONE;
-}
-}
-
-if (len >= sizeof(buffer)) {
-char *buf = PyMem_Malloc(len);
-if (buf == NULL)
-return PyErr_NoMemory();
-confstr(name, buf, len);
-result = PyUnicode_DecodeFSDefaultAndSize(buf, len-1);
-PyMem_Free(buf);
+result = Py_None;
+Py_INCREF(Py_None);
+}
 }
 else
-result = PyUnicode_DecodeFSDefaultAndSize(buffer, len-1);
+result = PyUnicode_DecodeFSDefaultAndSize(recvbuf, len-1);
+finally:
+PyMem_Free(allocated);
 return result;
 }
 #endif
# HG changeset patch
# Parent 5754f069b12342acb69dff739782416092522748

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -14372,32 +14372,35 @@ os_confstr_impl(PyModuleDef *module, int
 {
 PyObject *result = NULL;
 char buffer[255];
+char *recvbuf = buffer;
+char *allocated = NULL;
+size_t buflen = sizeof(buffer);
 size_t len;
 
-errno = 0;
-len = confstr(name, buffer, sizeof(buffer));
+/* Reset errno before each call as we may need to check it afterwards */
+while (errno = 0, (len = confstr(name, recvbuf, buflen)) > buflen) {
+recvbuf = PyMem_Realloc(allocated, len);
+if (recvbuf == NULL) {
+PyErr_NoMemory();
+goto finally;
+}
+allocated = recvbuf;

[issue9647] os.confstr() does not handle value changing length between calls

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I don't think that assert() is appropriate solution here. assert() is used to 
check internal logic, it shouldn't check conditions which rely on external 
world. Raising RuntimeError looks more appropriate to me.

--
versions:  -Python 2.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[David Watson]

David Watson added the comment:

On Fri 5 Dec 2014, STINNER Victor wrote:
> I added an assertion. Can we close this issue?

Well, if no one complains about the interpreter dying with
SIGABRT, that will suggest that the worries about OS bugs
creating infinite loops were unfounded :)

If you do want to leave this open, I can provide patches based on
the original one from issue #9579.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[STINNER Victor]

STINNER Victor added the comment:

(Oops, I didn't want to close the issue.)

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[STINNER Victor]

STINNER Victor added the comment:

I added an assertion. Can we close this issue?

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[Roundup Robot]

Roundup Robot added the comment:

New changeset a7a8947e9ce4 by Victor Stinner in branch 'default':
Issue #9647: os.confstr() ensures that the second call to confstr() returns the
https://hg.python.org/cpython/rev/a7a8947e9ce4

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I agree with Victor that two calls to confstr() should be enough. An example in 
confstr manpage uses two calls and I think there is no many software (if any) 
in the world which does more.

--
keywords: +easy
nosy: +serhiy.storchaka
stage:  -> needs patch
versions: +Python 3.4, Python 3.5 -Python 3.1, Python 3.2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[David Watson]

David Watson  added the comment:

> If I understood correctly, you don't want the value to be truncated if the 
> variable grows between the two calls to confstr(). Which behaviour would you 
> expect? A Python exception?

A return size larger than the buffer is *supposed* to indicate
that the current value is larger than the supplied buffer, so I
would just expect it to reallocate the buffer, call confstr()
again and return the new value, unless it was known that such a
situation indicated an actual problem.

In other words, I would not expect it to do anything special.  I
didn't write the original patch the way I did in order to fix
this (potential) bug - it just seemed like the most natural way
to write the code.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[STINNER Victor]

STINNER Victor  added the comment:

If I understood correctly, you don't want the value to be truncated if the 
variable grows between the two calls to confstr(). Which behaviour would you 
expect? A Python exception?

> but Victor Stinner has expressed concern that a buggy
> confstr() could create a near-infinite loop with that patch
> applied

Yes, I think that two calls to confstr() should be enough.

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9647] os.confstr() does not handle value changing length between calls

[David Watson]

New submission from David Watson :

This came up in relation to issue #9579; there is some discussion
of it there.  Basically, if os.confstr() has to call confstr()
twice because the buffer wasn't big enough the first time, the
existing code assumes the string is the same length that the OS
reported in the first call instead of using the length from the
second call and resizing the buffer if necessary.  This means the
returned value will be truncated or contain trailing garbage if
the string changed its length betweeen calls.

I don't know of an actual environment where configuration strings
can change at runtime, but it's not forbidden by POSIX as far as
I can see (the strings are described as "variables", after all,
and sysconf() values such as CHILD_MAX can change at runtime).
Implementations can also provide additional confstr() variables
not specified by POSIX.

The patch confstr-long-result.diff at issue #9579 would fix this
(for 3.x), but Victor Stinner has expressed concern that a buggy
confstr() could create a near-infinite loop with that patch
applied.

--
components: Extension Modules
messages: 114396
nosy: baikie
priority: normal
severity: normal
status: open
title: os.confstr() does not handle value changing length between calls
type: behavior
versions: Python 2.6, Python 2.7, Python 3.1, Python 3.2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com