msg64234 - (view) |
Author: Rolland Dudemaine (rolland) |
Date: 2008-03-21 10:27 |
In many files, the following code is present (with slight variations,
but the important part is there) :
static PyObject *
objargs_mktuple(va_list va)
{
int i, n = 0;
va_list countva;
PyObject *result, *tmp;
#ifdef VA_LIST_IS_ARRAY
memcpy(countva, va, sizeof(va_list));
#else
#ifdef __va_copy
__va_copy(countva, va);
#else
countva = va;
#endif
#endif
...
memcpy() is accessing va_list before it is initialized.
Before the first access to a va_list type variable, and after the last
access to that variable, calls to va_start() and va_end() must be made
to initialize and free the variable.
Such behaviour should be corrected in the following files :
- Objects/abstract.c, line 1901
- Objects/stringobject.c, line 162
- getargs.c, line 66
- getargs.c, line 1188
- modsupport.c, line 479
|
msg64250 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2008-03-21 18:26 |
Can you provide a patch for 2.6 against the latest svn checkout of the
trunk please?
|
msg64483 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2008-03-25 14:30 |
This is not a bug. All the reported functions expect va_list argument
to be initialized before being called. AFAICT, they are consistently
used in this way. For example,
PyObject *
PyObject_CallFunctionObjArgs(PyObject *callable, ...)
{
PyObject *args, *tmp;
va_list vargs;
if (callable == NULL)
return null_error();
/* count the args */
va_start(vargs, callable);
args = objargs_mktuple(vargs);
va_end(vargs);
if (args == NULL)
return NULL;
tmp = PyObject_Call(callable, args, NULL);
Py_DECREF(args);
return tmp;
}
This may need to be clarified in the docs. For example, PyString_FromFormatV does not mention that vargs needs to be
initialized: <http://docs.python.org/dev/c-
api/string.html#PyString_FromFormatV>. On the other hand, this may be
obvious to most C programmers.
I suggest to close this issue as invalid.
|
msg64487 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2008-03-25 15:14 |
On the second thought the macro dance highlighted by OP belongs to
pyport.h. Attached patch defines Py_VA_COPY macro and uses it to simplify
va_list copying code.
|
msg64500 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2008-03-25 17:23 |
Looks like a good idea to me
|
msg64523 - (view) |
Author: Rolland Dudemaine (rolland) |
Date: 2008-03-25 21:40 |
This is what I meant. The initialization should be done by calling
va_start(count_va); as you described.
In the files and lines I reported though, this is not called.
I'll file a patch for it soon.
--Rolland Dudemaine
Alexander Belopolsky wrote:
> Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:
>
> This is not a bug. All the reported functions expect va_list argument
> to be initialized before being called. AFAICT, they are consistently
> used in this way. For example,
>
> PyObject *
> PyObject_CallFunctionObjArgs(PyObject *callable, ...)
> {
> PyObject *args, *tmp;
> va_list vargs;
>
> if (callable == NULL)
> return null_error();
>
> /* count the args */
> va_start(vargs, callable);
> args = objargs_mktuple(vargs);
> va_end(vargs);
> if (args == NULL)
> return NULL;
> tmp = PyObject_Call(callable, args, NULL);
> Py_DECREF(args);
>
> return tmp;
> }
>
> This may need to be clarified in the docs. For example, PyString_FromFormatV does not mention that vargs needs to be
> initialized: <http://docs.python.org/dev/c-
> api/string.html#PyString_FromFormatV>. On the other hand, this may be
> obvious to most C programmers.
>
> I suggest to close this issue as invalid.
>
> ----------
> nosy: +belopolsky
>
> __________________________________
> Tracker <report@bugs.python.org>
> <http://bugs.python.org/issue2443>
> __________________________________
>
|
msg64588 - (view) |
Author: Rolland Dudemaine (rolland) |
Date: 2008-03-27 13:05 |
Actually, this thing is more complex to solve than I thought.
Specifically, as described in
http://www.opengroup.org/onlinepubs/007908775/xsh/stdarg.h.html stdarg
requires that variable argument functions have at least one fixed argument.
This is implied by the declaration of "void va_start(va_list ap, argN);".
As explained in the original ticket description, and also described
before in the above link, va_start() must be called before any call to
va_arg(), and this includes any access to the argument list using
__va_copy namely.
The problem is that at least objargs_mktuple(), line 2649 of
Objects/abstract.c does not have a first fixed argument.
|
msg70468 - (view) |
Author: Benjamin Peterson (benjamin.peterson) *  |
Date: 2008-07-31 02:03 |
What's the status of this?
|
msg84605 - (view) |
Author: Daniel Stutzbach (stutzbach)  |
Date: 2009-03-30 18:28 |
Rolland,
The va_list is initialized by the function that calls objargs_mktuple.
va_start() and va_end() need to be called in the function that takes
"..." as a parameter, and it is.
Not a bug, but +1 on Alexander's patch to consolidate all the #ifdef's
for cleanliness.
|
msg88611 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2009-05-31 21:29 |
Reducing the priority and updating the target releases, since from the
discussion there doesn't appear to be a bug here.
|
msg99987 - (view) |
Author: Alexander Belopolsky (Alexander.Belopolsky) |
Date: 2010-02-24 01:27 |
Updated the patch. In a year one more copy of va_copy macro dance have cropped in. Any reason this is still languishing?
|
msg99989 - (view) |
Author: Alexander Belopolsky (Alexander.Belopolsky) |
Date: 2010-02-24 01:33 |
Rolland,
Please don't revert the title. The consensus is that there is no uninitialized access to va_list.
|
msg113597 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2010-08-11 09:27 |
Looks like a good idea. Don't other compilers have __va_copy equivalents?
Apparently, C99 defines va_copy(), which we could use conditionally.
|
msg113608 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2010-08-11 16:56 |
I updated the patch for 3.x. I agree that using va_copy where available makes sense, but let's leave this type of improvements for the future.
|
msg113635 - (view) |
Author: Alexander Belopolsky (belopolsky) *  |
Date: 2010-08-11 22:12 |
Committed in r83949.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:32 | admin | set | github: 46695 |
2010-08-11 22:12:59 | belopolsky | set | status: open -> closed
messages:
+ msg113635 stage: commit review -> resolved |
2010-08-11 16:56:17 | belopolsky | set | files:
+ issue2443-py3k.diff
messages:
+ msg113608 |
2010-08-11 09:27:28 | pitrou | set | nosy:
+ pitrou messages:
+ msg113597
|
2010-08-11 00:41:22 | belopolsky | set | assignee: belopolsky
nosy:
- Alexander.Belopolsky |
2010-08-09 18:37:49 | terry.reedy | set | versions:
- Python 2.7 |
2010-02-24 01:33:30 | Alexander.Belopolsky | set | messages:
+ msg99989 title: uninitialized access to va_list -> Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy |
2010-02-24 01:27:24 | Alexander.Belopolsky | set | files:
+ issue2443a.diff nosy:
+ Alexander.Belopolsky messages:
+ msg99987
|
2009-05-31 21:29:18 | r.david.murray | set | priority: critical -> normal versions:
+ Python 2.7, Python 3.2, - Python 2.6, Python 2.5, Python 3.0 nosy:
+ r.david.murray
messages:
+ msg88611
stage: commit review |
2009-03-30 18:28:47 | stutzbach | set | nosy:
+ stutzbach messages:
+ msg84605
|
2008-09-04 01:14:41 | benjamin.peterson | set | priority: release blocker -> critical |
2008-08-21 14:57:52 | benjamin.peterson | set | priority: critical -> release blocker |
2008-07-31 02:03:53 | benjamin.peterson | set | nosy:
+ benjamin.peterson messages:
+ msg70468 |
2008-03-27 13:05:27 | rolland | set | messages:
+ msg64588 |
2008-03-25 21:40:20 | rolland | set | messages:
+ msg64523 title: Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy -> uninitialized access to va_list |
2008-03-25 18:05:49 | belopolsky | set | type: compile error -> enhancement title: uninitialized access to va_list -> Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy |
2008-03-25 17:23:36 | christian.heimes | set | priority: high -> critical resolution: accepted messages:
+ msg64500 |
2008-03-25 15:14:36 | belopolsky | set | files:
+ issue2443.diff keywords:
+ patch messages:
+ msg64487 |
2008-03-25 14:30:58 | belopolsky | set | nosy:
+ belopolsky messages:
+ msg64483 |
2008-03-21 18:26:03 | christian.heimes | set | priority: high nosy:
+ christian.heimes messages:
+ msg64250 components:
+ Interpreter Core, - Build |
2008-03-21 10:27:37 | rolland | create | |