Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy

[rolland]

BPO	2443
Nosy	@abalkin, @pitrou, @tiran, @benjaminp, @bitdancer
Files	issue2443.diff: Patch against revision 61893 issue2443a.diff: Patch against revision 78398 issue2443-py3k.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/abalkin'
closed_at = <Date 2010-08-11.22:12:59.423>
created_at = <Date 2008-03-21.10:27:37.295>
labels = ['interpreter-core', 'type-feature']
title = 'Define Py_VA_COPY macro as a cross-platform replacement for gcc __va_copy'
updated_at = <Date 2010-08-11.22:12:59.422>
user = 'https://bugs.python.org/rolland'

bugs.python.org fields:

activity = <Date 2010-08-11.22:12:59.422>
actor = 'belopolsky'
assignee = 'belopolsky'
closed = True
closed_date = <Date 2010-08-11.22:12:59.423>
closer = 'belopolsky'
components = ['Interpreter Core']
creation = <Date 2008-03-21.10:27:37.295>
creator = 'rolland'
dependencies = []
files = ['9849', '16349', '18477']
hgrepos = []
issue_num = 2443
keywords = ['patch']
message_count = 15.0
messages = ['64234', '64250', '64483', '64487', '64500', '64523', '64588', '70468', '84605', '88611', '99987', '99989', '113597', '113608', '113635']
nosy_count = 7.0
nosy_names = ['belopolsky', 'pitrou', 'christian.heimes', 'benjamin.peterson', 'stutzbach', 'rolland', 'r.david.murray']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue2443'
versions = ['Python 3.2']

[rolland]

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

[tiran]

Can you provide a patch for 2.6 against the latest svn checkout of the
trunk please?

[abalkin]

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.

[abalkin]

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.

[tiran]

Looks like a good idea to me

[rolland]

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\>

---

[rolland]

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.

[benjaminp]

What's the status of this?

[stutzbach]

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.

[bitdancer]

Reducing the priority and updating the target releases, since from the
discussion there doesn't appear to be a bug here.

[AlexanderBelopolsky]

Updated the patch. In a year one more copy of va_copy macro dance have cropped in. Any reason this is still languishing?

[AlexanderBelopolsky]

Rolland,

Please don't revert the title. The consensus is that there is no uninitialized access to va_list.

[pitrou]

Looks like a good idea. Don't other compilers have __va_copy equivalents?
Apparently, C99 defines va_copy(), which we could use conditionally.

[abalkin]

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.

[abalkin]

Committed in r83949.