bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size() #1096

serhiy-storchaka · 2017-04-12T21:14:53Z

raised an error.

Replace them with using concrete types API that never fails if appropriate.

…) raised an error. Replace them with using concrete types API that never fails if appropriate.

zhangyangyu · 2017-04-14T05:33:18Z

Modules/_winapi.c

    keys = PyMapping_Keys(environment);
    values = PyMapping_Values(environment);
    if (!keys || !values)
        goto error;

+    envsize = PyList_GET_SIZE(keys);


In this function, PyList_GET_ITEM is used. But PyMapping_Keys/Values could also return a tuple. Is it possible here?

This is a separate issue. See bpo-28280.

I have replaced PyList_GET_ITEM by PySequence_Fast_GET_ITEM.

zhangyangyu · 2017-04-14T05:37:59Z

Modules/cjkcodecs/multibytecodec.c

@@ -1670,6 +1670,9 @@ _multibytecodec_MultibyteStreamWriter_writelines(MultibyteStreamWriterObject *se
        if (r == -1)
            return NULL;
    }
+    /* PySequence_Length() can fail */
+    if (PyErr_Occurred())


Maybe cache the result of PySequence_Length. It's not free. And even lines is changed the behaviour is not bad.

This would be a behavior change. Since I'm going to backport most of these changes I prefer to keep the current behavior.

zhangyangyu · 2017-04-14T05:52:14Z

Modules/posixmodule.c

+                return NULL;
+            }
+            sf.hdr_cnt = (int)i;
+            i = 0;


This i = 0 looks not needed.

Indeed. if sf.hdr_cnt > 0 is false, i can only be 0.

zhangyangyu · 2017-04-14T06:01:29Z

Objects/setobject.c

@@ -1559,7 +1559,9 @@ set_difference(PySetObject *so, PyObject *other)

    /* If len(so) much more than len(other), it's more efficient to simply copy
     * so and then iterate other looking for common elements. */
-    if ((PySet_GET_SIZE(so) >> 2) > PyObject_Size(other)) {
+    other_size = PyDict_CheckExact(other) ? PyDict_GET_SIZE(other)
+                                          : PySet_GET_SIZE(other);


I prefer:

if (PyAnySet_Check(other) { other_size = PySet_GET_SIZE(other); } else if (PyDict_CheckExact(other)) { other_size = PyDict_GET_SIZE(other); } else { return set_copy_and_difference(so, other); }

I like this too. But let ask @rhettinger.

zhangyangyu · 2017-04-14T06:04:22Z

Modules/posixmodule.c

+                return NULL;
+            }
+            sf.trl_cnt = (int)i;
+            i = 0;


zhangyangyu · 2017-04-16T12:30:14Z

Modules/_io/iobase.c

-
-        if (length > hint)
+        if (line_length < 0) {
+            Py_DECREF(result);


Should be goto error; here.

Good catch. Now we need to clear the iterator too.

zhangyangyu · 2017-04-16T12:36:10Z

Modules/_io/iobase.c

@@ -671,6 +672,7 @@ _io__IOBase_readlines_impl(PyObject *self, Py_ssize_t hint)

    while (1) {
        PyObject *line = PyIter_Next(it);
+        Py_ssize_t line_length;


Better move this line before the above. IIRC C89 doesn't allow mixing variable declaration and assignment.

There is no mixing variable declaration and assignment. PyObject *line = PyIter_Next(it); is not an assignment, it is an initialization. But I'll swap the lines because this makes line initialization closer to checking it's value.

zhangyangyu · 2017-04-19T16:38:51Z

Modules/posixmodule.c

                return NULL;
+            }
+            if (i > 0) {
+                sf.hdr_cnt = (int)i;


Re-scanning the patch I get a question here. Before PySequence_Size(headers) returns 0 and sf.hdr_cnt is set. Now when it returns 0, sf.hdr_cnt is uninitialized. Codes below still use sf but I don't know sf.hdr_cnt matters or not.

This doesn't differ from the case when arguments headers or trailers are not specified. sf.headers and sf.trailers are set to NULL and according to the documentation this should be enough.

I mean ret = sendfile(in, out, offset, &sbytes, &sf, flags). Is there any guarantee this function won't depend on sf.hdr_cnt? Could it be just a for loop and then cause dereferencing a NULL pointer?

python#1096) raised an error. Replace them with using concrete types API that never fails if appropriate. (cherry picked from commit bf623ae)

#1096) (#1180) raised an error. (cherry picked from commit bf623ae)

python#1096) (python#1180) raised an error. (cherry picked from commit bf623ae) (cherry picked from commit 680fea4)

…_Size() (pythonGH-1096) (pythonGH-1180) raised an error. (cherry picked from commit bf623ae). (cherry picked from commit 680fea4)

…_Size() (pythonGH-1096) (pythonGH-1180) raised an error. (cherry picked from commit bf623ae) (cherry picked from commit 680fea4)

…_Size() (pythonGH-1096) (pythonGH-1180) raised an error. (cherry picked from commit bf623ae). (cherry picked from commit 680fea4)

…_Size() (GH-1096) (GH-1180) (#1182) raised an error. (cherry picked from commit bf623ae) (cherry picked from commit 680fea4)

…_Size() (GH-1096) (GH-1180) (#1183) raised an error. (cherry picked from commit bf623ae) (cherry picked from commit 680fea4)

bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size(…

8317163

…) raised an error. Replace them with using concrete types API that never fails if appropriate.

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 2.7 labels Apr 12, 2017

the-knights-who-say-ni added the CLA signed label Apr 12, 2017

zhangyangyu reviewed Apr 14, 2017

View reviewed changes

Modules/posixmodule.c Outdated

return NULL;

}

sf.trl_cnt = (int)i;

i = 0;

Copy link

Member

zhangyangyu Apr 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

serhiy-storchaka added 3 commits April 14, 2017 10:15

Address Xiang's comments.

68b90d8

Add additional test.

80e16a2

Merge branch 'master' into PyObject_Size-check-error

5b6e6ba

zhangyangyu reviewed Apr 16, 2017

View reviewed changes

serhiy-storchaka added 3 commits April 16, 2017 19:22

Fix a leak in iobase.c.

e459aa6

Add a Misc/NEWS entry and optimize set checks.

25cc5b5

Fixed a syntax error.

6908bd9

zhangyangyu reviewed Apr 19, 2017

View reviewed changes

serhiy-storchaka merged commit bf623ae into python:master Apr 19, 2017

serhiy-storchaka deleted the PyObject_Size-check-error branch April 19, 2017 17:03

serhiy-storchaka added a commit that referenced this pull request Apr 19, 2017

bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size() (

680fea4

#1096) (#1180) raised an error. (cherry picked from commit bf623ae)

serhiy-storchaka added a commit that referenced this pull request Apr 19, 2017

[3.5] bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping…

e63af29

…_Size() (GH-1096) (GH-1180) (#1182) raised an error. (cherry picked from commit bf623ae) (cherry picked from commit 680fea4)

serhiy-storchaka added a commit that referenced this pull request Apr 19, 2017

[2.7] bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping…

64aa4df

…_Size() (GH-1096) (GH-1180) (#1183) raised an error. (cherry picked from commit bf623ae) (cherry picked from commit 680fea4)

Mariatta removed needs backport to 2.7 labels Apr 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size() #1096

bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size() #1096

serhiy-storchaka commented Apr 12, 2017

zhangyangyu Apr 14, 2017

serhiy-storchaka Apr 14, 2017

zhangyangyu Apr 14, 2017 •

edited

Loading

serhiy-storchaka Apr 14, 2017

zhangyangyu Apr 14, 2017

serhiy-storchaka Apr 14, 2017

zhangyangyu Apr 14, 2017 •

edited

Loading

serhiy-storchaka Apr 14, 2017

zhangyangyu Apr 14, 2017

zhangyangyu Apr 16, 2017

serhiy-storchaka Apr 16, 2017

zhangyangyu Apr 16, 2017

serhiy-storchaka Apr 16, 2017

zhangyangyu Apr 19, 2017

serhiy-storchaka Apr 19, 2017

zhangyangyu Apr 19, 2017

bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size() #1096

bpo-30061: Check if PyObject_Size()/PySequence_Size()/PyMapping_Size() #1096

Conversation

serhiy-storchaka commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyangyu Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyangyu Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyangyu Apr 14, 2017 •

edited

Loading

zhangyangyu Apr 14, 2017 •

edited

Loading