[Python-Dev] [Python-checkins] cpython (3.2): Fixes issue #8052: The posix subprocess module's close_fds behavior was

[Gregory P. Smith]

On Sat, Jan 21, 2012 at 4:21 PM, Benjamin Peterson <benjamin at python.org> wrote:
> 2012/1/21 gregory.p.smith <python-checkins at python.org>:
> ...
>> +/* Convert ASCII to a positive int, no libc call. no overflow. -1 on error. */
>
> Is no libc call important?

Yes.  strtol() is not on the async signal safe C library function list.

>
>> +static int _pos_int_from_ascii(char *name)
>
> To be consistent with the rest of posixmodule.c, "static int" should
> be on a different line from the signature. This also applies to all
> other function declarations added by this.

Python C style as a whole, yes.  This file already has a mix of same
line vs two line declarations, I added these following the style of
the functions immediately surrounding them.  Want a style fixup on the
whole file?

>
>> +{
>> +    int num = 0;
>> +    while (*name >= '0' && *name <= '9') {
>> +        num = num * 10 + (*name - '0');
>> +        ++name;
>> +    }
>> +    if (*name)
>> +        return -1;  /* Non digit found, not a number. */
>> +    return num;
>> +}
>> +
>> +
>> +/* Returns 1 if there is a problem with fd_sequence, 0 otherwise. */
>> +static int _sanity_check_python_fd_sequence(PyObject *fd_sequence)
>> +{
>> +    Py_ssize_t seq_idx, seq_len = PySequence_Length(fd_sequence);
>
> PySequence_Length can fail.

It has already been checked not to by the only entry point into the
code in this file.

>
>> +    long prev_fd = -1;
>> +    for (seq_idx = 0; seq_idx < seq_len; ++seq_idx) {
>> +        PyObject* py_fd = PySequence_Fast_GET_ITEM(fd_sequence, seq_idx);
>> +        long iter_fd = PyLong_AsLong(py_fd);
>> +        if (iter_fd < 0 || iter_fd < prev_fd || iter_fd > INT_MAX) {
>> +            /* Negative, overflow, not a Long, unsorted, too big for a fd. */
>> +            return 1;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +
>> +/* Is fd found in the sorted Python Sequence? */
>> +static int _is_fd_in_sorted_fd_sequence(int fd, PyObject *fd_sequence)
>> +{
>> +    /* Binary search. */
>> +    Py_ssize_t search_min = 0;
>> +    Py_ssize_t search_max = PySequence_Length(fd_sequence) - 1;
>> +    if (search_max < 0)
>> +        return 0;
>> +    do {
>> +        long middle = (search_min + search_max) / 2;
>> +        long middle_fd = PyLong_AsLong(
>> +                PySequence_Fast_GET_ITEM(fd_sequence, middle));
>
> No check for error?

_sanity_check_python_fd_sequence() already checked the entire list to
guarantee that there would not be any such error.

>> +        if (fd == middle_fd)
>> +            return 1;
>> +        if (fd > middle_fd)
>> +            search_min = middle + 1;
>> +        else
>> +            search_max = middle - 1;
>> +    } while (search_min <= search_max);
>> +    return 0;
>> +}

In general this is an extension module that is best viewed as a whole
including its existing comments rather than as a diff.

It contains code that will look "odd" in a diff because much of it
executes in a path where not much is allowed (post fork, pre exec) and
no useful way of responding to an error is possible so it attempts to
pre-check for any possible errors up front so that later code that is
unable to handle errors cannot possibly fail.

-gps