Skip to content

Commit b9d24b1

Browse files
fangererfangerer
committed
[GR-31918][GR-31952] Fix: __setitem__ should not mutate deque state.
PullRequest: graalpython/1838
2 parents 895a103 + 7d1fb99 commit b9d24b1

File tree

7 files changed

+92
-37
lines changed

7 files changed

+92
-37
lines changed

‎graalpython/com.oracle.graal.python.cext/src/structseq.c‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,16 +73,22 @@ int PyStructSequence_InitType2(PyTypeObject *type, PyStructSequence_Desc *desc)
7373
PyObject* field_docs = PyTuple_New(n_members);
7474
PyStructSequence_Field* fields = desc->fields;
7575
for (i = 0; i < n_members; i++) {
76-
PyTuple_SetItem(field_names, i, polyglot_from_string(fields[i].name, SRC_CS));
77-
PyTuple_SetItem(field_docs, i, polyglot_from_string(fields[i].doc, SRC_CS));
76+
PyTuple_SetItem(field_names, i, polyglot_from_string(fields[i].name, SRC_CS));
77+
PyTuple_SetItem(field_docs, i, polyglot_from_string(fields[i].doc, SRC_CS));
7878
}
7979

8080
// we create the new type managed
8181
PyTypeObject* newType = (PyTypeObject*) UPCALL_CEXT_O(_jls_PyStructSequence_InitType2,
82-
polyglot_from_string(desc->name, SRC_CS),
83-
polyglot_from_string(desc->doc, SRC_CS),
84-
native_to_java(field_names),
85-
native_to_java(field_docs));
82+
polyglot_from_string(desc->name, SRC_CS),
83+
polyglot_from_string(desc->doc, SRC_CS),
84+
native_to_java(field_names),
85+
native_to_java(field_docs));
86+
87+
if (newType == NULL) {
88+
Py_DECREF(field_names);
89+
Py_DECREF(field_docs);
90+
return -1;
91+
}
8692

8793
// copy generic fields (CPython mem-copies a template)
8894
type->tp_basicsize = sizeof(PyStructSequence) - sizeof(PyObject *);

‎graalpython/com.oracle.graal.python.test/src/tests/unittest_tags/test_deque.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
*graalpython.lib-python.3.test.test_deque.TestBasic.test_init
2727
*graalpython.lib-python.3.test.test_deque.TestBasic.test_insert
2828
*graalpython.lib-python.3.test.test_deque.TestBasic.test_insert_bug_26194
29+
*graalpython.lib-python.3.test.test_deque.TestBasic.test_iterator_pickle
2930
*graalpython.lib-python.3.test.test_deque.TestBasic.test_len
3031
*graalpython.lib-python.3.test.test_deque.TestBasic.test_long_steadystate_queue_popleft
3132
*graalpython.lib-python.3.test.test_deque.TestBasic.test_long_steadystate_queue_popright
@@ -68,7 +69,10 @@
6869
*graalpython.lib-python.3.test.test_deque.TestSequence.test_truth
6970
*graalpython.lib-python.3.test.test_deque.TestSubclass.test_basics
7071
*graalpython.lib-python.3.test.test_deque.TestSubclass.test_bug_31608
72+
*graalpython.lib-python.3.test.test_deque.TestSubclass.test_copy_pickle
73+
*graalpython.lib-python.3.test.test_deque.TestSubclass.test_pickle_recursive
7174
*graalpython.lib-python.3.test.test_deque.TestSubclass.test_strange_subclass
75+
*graalpython.lib-python.3.test.test_deque.TestSubclass.test_weakref
7276
*graalpython.lib-python.3.test.test_deque.TestSubclassWithKwargs.test_subclass_with_kwargs
7377
*graalpython.lib-python.3.test.test_deque.TestVariousIteratorArgs.test_constructor
7478
*graalpython.lib-python.3.test.test_deque.TestVariousIteratorArgs.test_iter_with_altered_data

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/deque/DequeBuiltins.java‎

Lines changed: 16 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import static com.oracle.graal.python.builtins.PythonBuiltinClassType.StopIteration;
4848
import static com.oracle.graal.python.builtins.PythonBuiltinClassType.TypeError;
4949
import static com.oracle.graal.python.builtins.PythonBuiltinClassType.ValueError;
50+
import static com.oracle.graal.python.nodes.SpecialAttributeNames.__DICT__;
5051
import static com.oracle.graal.python.nodes.SpecialMethodNames.__ADD__;
5152
import static com.oracle.graal.python.nodes.SpecialMethodNames.__CONTAINS__;
5253
import static com.oracle.graal.python.nodes.SpecialMethodNames.__DELITEM__;
@@ -74,6 +75,7 @@
7475
import com.oracle.graal.python.annotations.ArgumentClinic.ClinicConversion;
7576
import com.oracle.graal.python.builtins.Builtin;
7677
import com.oracle.graal.python.builtins.CoreFunctions;
78+
import com.oracle.graal.python.builtins.Python3Core;
7779
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
7880
import com.oracle.graal.python.builtins.PythonBuiltins;
7981
import com.oracle.graal.python.builtins.objects.PNone;
@@ -93,6 +95,8 @@
9395
import com.oracle.graal.python.builtins.objects.type.TypeNodes.GetNameNode;
9496
import com.oracle.graal.python.lib.PyNumberAsSizeNode;
9597
import com.oracle.graal.python.lib.PyObjectIsTrueNode;
98+
import com.oracle.graal.python.lib.PyObjectLookupAttr;
99+
import com.oracle.graal.python.lib.PyObjectSizeNode;
96100
import com.oracle.graal.python.nodes.ErrorMessages;
97101
import com.oracle.graal.python.nodes.PGuards;
98102
import com.oracle.graal.python.nodes.PRaiseNode;
@@ -117,7 +121,6 @@
117121
import com.oracle.graal.python.nodes.util.CastToJavaIntExactNode;
118122
import com.oracle.graal.python.nodes.util.CastToJavaStringNode;
119123
import com.oracle.graal.python.runtime.PythonContext;
120-
import com.oracle.graal.python.builtins.Python3Core;
121124
import com.oracle.graal.python.runtime.exception.PException;
122125
import com.oracle.graal.python.runtime.object.PythonObjectFactory;
123126
import com.oracle.truffle.api.CompilerDirectives;
@@ -269,8 +272,9 @@ static PNone doGeneric(PDeque self) {
269272
public abstract static class DequeCopyNode extends PythonUnaryBuiltinNode {
270273

271274
@Specialization
272-
PDeque doGeneric(PDeque self) {
273-
PDeque copy = factory().createDeque();
275+
PDeque doGeneric(PDeque self,
276+
@Cached GetClassNode getClassNode) {
277+
PDeque copy = factory().createDeque(getClassNode.execute(self));
274278
copy.setMaxLength(self.getMaxLength());
275279
copy.addAll(self);
276280
return copy;
@@ -853,31 +857,9 @@ protected ArgumentClinicProvider getArgumentClinic() {
853857
static PNone doGeneric(PDeque self, int idx, Object value,
854858
@Cached NormalizeIndexCustomMessageNode normalizeIndexNode) {
855859
int normIdx = normalizeIndexNode.execute(idx, self.getSize(), ErrorMessages.DEQUE_INDEX_OUT_OF_RANGE);
856-
doSetItem(self, normIdx, value);
860+
self.setItem(normIdx, value != PNone.NO_VALUE ? value : null);
857861
return PNone.NONE;
858862
}
859-
860-
@TruffleBoundary
861-
static void doSetItem(PDeque self, int idx, Object value) {
862-
assert 0 <= idx && idx < self.getSize();
863-
int n = self.getSize() - idx - 1;
864-
Object[] savedItems = new Object[n];
865-
for (int i = 0; i < savedItems.length; i++) {
866-
savedItems[i] = self.pop();
867-
}
868-
// this removes the item we want to replace
869-
self.pop();
870-
assert self.getSize() == idx;
871-
if (value != PNone.NO_VALUE) {
872-
self.append(value);
873-
}
874-
875-
// re-add saved items
876-
for (int i = savedItems.length - 1; i >= 0; i--) {
877-
self.append(savedItems[i]);
878-
}
879-
assert value != PNone.NO_VALUE && self.getSize() == n + idx + 1 || value == PNone.NO_VALUE && self.getSize() == n + idx;
880-
}
881863
}
882864

883865
@Builtin(name = __DELITEM__, minNumOfPositionalArgs = 2, parameterNames = {"$self", "n"})
@@ -894,7 +876,7 @@ protected ArgumentClinicProvider getArgumentClinic() {
894876
static PNone doGeneric(PDeque self, int idx,
895877
@Cached NormalizeIndexCustomMessageNode normalizeIndexNode) {
896878
int normIdx = normalizeIndexNode.execute(idx, self.getSize(), ErrorMessages.DEQUE_INDEX_OUT_OF_RANGE);
897-
DequeSetItemNode.doSetItem(self, normIdx, PNone.NO_VALUE);
879+
self.setItem(normIdx, null);
898880
return PNone.NONE;
899881
}
900882
}
@@ -966,12 +948,17 @@ static Object repr(PDeque self) {
966948
abstract static class DequeReduceNode extends PythonUnaryBuiltinNode {
967949

968950
@Specialization(limit = "1")
969-
Object doGeneric(PDeque self,
951+
Object doGeneric(VirtualFrame frame, PDeque self,
970952
@CachedLibrary("self") PythonObjectLibrary lib,
953+
@Cached PyObjectLookupAttr lookupAttr,
954+
@Cached PyObjectSizeNode sizeNode,
971955
@Cached GetClassNode getClassNode,
972956
@Cached ConditionProfile profile) {
973957
Object clazz = getPythonClass(getClassNode.execute(self), profile);
974-
Object dict = lib.hasDict(self) ? lib.getDict(self) : PNone.NONE;
958+
Object dict = lookupAttr.execute(frame, self, __DICT__);
959+
if (PGuards.isNoValue(dict) || sizeNode.execute(frame, dict) <= 0) {
960+
dict = PNone.NONE;
961+
}
975962
Object it = lib.getIterator(self);
976963
PTuple emptyTuple = factory().createEmptyTuple();
977964
int maxLength = self.getMaxLength();

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/deque/PDeque.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,32 @@ public void clear() {
166166
state++;
167167
}
168168

169+
@TruffleBoundary
170+
public void setItem(int idx, Object value) {
171+
assert 0 <= idx && idx < data.size();
172+
int n = data.size() - idx - 1;
173+
Object[] savedItems = new Object[n];
174+
for (int i = 0; i < savedItems.length; i++) {
175+
savedItems[i] = data.pollLast();
176+
}
177+
// this removes the item we want to replace
178+
data.pollLast();
179+
assert data.size() == idx;
180+
if (value != null) {
181+
data.addLast(value);
182+
} else {
183+
// removal case: this alters the number of elements, so modify the state
184+
state++;
185+
}
186+
187+
// re-add saved items
188+
for (int i = savedItems.length - 1; i >= 0; i--) {
189+
data.addLast(savedItems[i]);
190+
}
191+
assert maxLength == -1 || data.size() <= maxLength;
192+
assert value != null && data.size() == n + idx + 1 || value == null && data.size() == n + idx;
193+
}
194+
169195
public int getState() {
170196
return state;
171197
}

‎graalpython/lib-graalpython/_collections.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,35 @@ def __reduce__(self):
5656

5757

5858
defaultdict.__module__ = 'collections'
59+
60+
61+
class _tuplegetter(object):
62+
@__graalpython__.builtin_method
63+
def __init__(self, index, doc):
64+
self.index = index
65+
self.__doc__ = doc
66+
67+
@__graalpython__.builtin_method
68+
def __set__(self, instance, value):
69+
raise AttributeError("can't set attribute")
70+
71+
@__graalpython__.builtin_method
72+
def __delete__(self, instance):
73+
raise AttributeError("can't delete attribute")
74+
75+
@__graalpython__.builtin_method
76+
def __get__(self, instance, owner=None):
77+
index = self.index
78+
if not isinstance(instance, tuple):
79+
if instance is None:
80+
return self
81+
raise TypeError("descriptor for index '%d' for tuple subclasses "
82+
"doesn't apply to '%s' object" % (index, instance))
83+
if index >= len(instance):
84+
raise IndexError("tuple index out of range")
85+
86+
return instance[index]
87+
88+
@__graalpython__.builtin_method
89+
def __reduce__(self):
90+
return type(self), (self.index, self.__doc__)

‎graalpython/lib-graalpython/python_cext.py‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -814,8 +814,7 @@ def PyStructSequence_InitType2(tp_name, type_doc, field_names, field_docs):
814814
new_type.__doc__ = type_doc
815815
for i in range(len(field_names)):
816816
prop = getattr(new_type, field_names[i])
817-
assert isinstance(prop, property)
818-
prop.__doc__ = field_docs[i]
817+
assert hasattr(prop, "__doc__")
819818
# ensure '_fields' attribute; required in 'PyStructSequence_New'
820819
assert hasattr(new_type, "_fields")
821820
return new_type

‎graalpython/lib-python/3/test/test_deque.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -896,6 +896,7 @@ def test_pickle_recursive(self):
896896
for d in DequeWithBadIter('abc'), DequeWithBadIter('abc', 2):
897897
self.assertRaises(TypeError, pickle.dumps, d, proto)
898898

899+
@support.impl_detail("finalization", graalvm=False)
899900
def test_weakref(self):
900901
d = deque('gallahad')
901902
p = weakref.proxy(d)

0 commit comments

Comments
 (0)