classification
Title: Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy
Type: enhancement Stage: committed/rejected
Components: Interpreter Core Versions: Python 3.2
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: belopolsky Nosy List: belopolsky, benjamin.peterson, christian.heimes, pitrou, r.david.murray, rolland, stutzbach
Priority: normal Keywords: patch

Created on 2008-03-21 10:27 by rolland, last changed 2010-08-11 22:12 by belopolsky. This issue is now closed.

Files
File name Uploaded Description Edit
issue2443.diff belopolsky, 2008-03-25 15:14 Patch against revision 61893
issue2443a.diff Alexander.Belopolsky, 2010-02-24 01:27 Patch against revision 78398
issue2443-py3k.diff belopolsky, 2010-08-11 16:56
Messages (15)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2008-07-31 02:03
What's the status of this?
msg84605 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-08-11 22:12
Committed in r83949.
History
Date User Action Args
2010-08-11 22:12:59belopolskysetstatus: open -> closed

messages: + msg113635
stage: commit review -> committed/rejected
2010-08-11 16:56:17belopolskysetfiles: + issue2443-py3k.diff

messages: + msg113608
2010-08-11 09:27:28pitrousetnosy: + pitrou
messages: + msg113597
2010-08-11 00:41:22belopolskysetassignee: belopolsky

nosy: - Alexander.Belopolsky
2010-08-09 18:37:49terry.reedysetversions: - Python 2.7
2010-02-24 01:33:30Alexander.Belopolskysetmessages: + 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:24Alexander.Belopolskysetfiles: + issue2443a.diff
nosy: + Alexander.Belopolsky
messages: + msg99987

2009-05-31 21:29:18r.david.murraysetpriority: 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:47stutzbachsetnosy: + stutzbach
messages: + msg84605
2008-09-04 01:14:41benjamin.petersonsetpriority: release blocker -> critical
2008-08-21 14:57:52benjamin.petersonsetpriority: critical -> release blocker
2008-07-31 02:03:53benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg70468
2008-03-27 13:05:27rollandsetmessages: + msg64588
2008-03-25 21:40:20rollandsetmessages: + 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:49belopolskysettype: 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:36christian.heimessetpriority: high -> critical
resolution: accepted
messages: + msg64500
2008-03-25 15:14:36belopolskysetfiles: + issue2443.diff
keywords: + patch
messages: + msg64487
2008-03-25 14:30:58belopolskysetnosy: + belopolsky
messages: + msg64483
2008-03-21 18:26:03christian.heimessetpriority: high
nosy: + christian.heimes
messages: + msg64250
components: + Interpreter Core, - Build
2008-03-21 10:27:37rollandcreate