[issue46615] Use-after-free by mutating set during set operations

[Dennis Sweeney]

Dennis Sweeney  added the comment:

It does look like there are some pickle situations that crash. Attached is a 
randomized crasher. I haven't done too much careful reasoning about it, but 
adding INCREFs everywhere seems to fix most of the issues.

--
Added file: https://bugs.python.org/file50631/picklecrasher.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Thanks Dennis for your report and PRs.

Do you mind to analyze also uses of _PySet_NextEntry(), PyDict_Next() and 
_PyDict_Next()? Many of them look safe, but _pickle.c seems vulnerable.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset c31b8a97a8a7e8255231c9e12ed581c6240c0d6c by Dennis Sweeney in 
branch '3.9':
bpo-46615: Don't crash when set operations mutate the sets (GH-31120) (GH-31312)
https://github.com/python/cpython/commit/c31b8a97a8a7e8255231c9e12ed581c6240c0d6c


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Dennis Sweeney]

Change by Dennis Sweeney :


--
pull_requests: +29473
pull_request: https://github.com/python/cpython/pull/31312

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[miss-islington]

miss-islington  added the comment:


New changeset 1f5fe9962f768c8bfd4ed06a22532d31d3424dc9 by Miss Islington (bot) 
in branch '3.10':
bpo-46615: Don't crash when set operations mutate the sets (GH-31120)
https://github.com/python/cpython/commit/1f5fe9962f768c8bfd4ed06a22532d31d3424dc9


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[miss-islington]

Change by miss-islington :


--
nosy: +miss-islington
nosy_count: 4.0 -> 5.0
pull_requests: +29444
pull_request: https://github.com/python/cpython/pull/31284

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Dennis Sweeney]

Dennis Sweeney  added the comment:


New changeset 4a66615ba736f84eadf9456bfd5d32a94cccf117 by Dennis Sweeney in 
branch 'main':
bpo-46615: Don't crash when set operations mutate the sets (GH-31120)
https://github.com/python/cpython/commit/4a66615ba736f84eadf9456bfd5d32a94cccf117


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Dennis Sweeney]

Change by Dennis Sweeney :


--
keywords: +patch
pull_requests: +29301
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/31120

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Tim Peters]

Tim Peters  added the comment:

Raised the priority back to normal.

I agree with Dennis's observation that PyDict_Next is safe, provided it's used 
as intended: it returns borrowed references, but to things that absolutely are 
legitimate at the time. In the presence of mutations, *what* it returns isn't 
defined at all, but I don't see a way for it to blow up (unless its caller 
screws up by believing it owns the references). It doesn't assume anything 
about the structure of the dict across calls.

--
nosy: +tim.peters
priority: low -> normal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Dennis Sweeney]

Dennis Sweeney  added the comment:

It looks like usages of the PyDict_Next API assume the resulting references are 
borrowed and so INCREF them.

Usages of set_next do not, but should.

It should hopefully be a straightforward fix of adding INCREF/DECREFs.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Raymond Hettinger]

Raymond Hettinger  added the comment:

Marking as low priority given that ehe next loop code has been deployed without 
incident for two decades (a little less for sets and a little more for dicts).

--
priority: normal -> low

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Raymond Hettinger]

Raymond Hettinger  added the comment:

Presumably _PyDict_Next is also suspect.  Even the advertised "safe" calls to 
PyDict_SetItem() for existing keys would be a trigger.  Calling clear() in 
either __eq__ or __hash__ would suffice.

If the next loops are the culprint, the new challenge is figuring out how to 
fix it without wrecking code clarity and performance (and having to deprecate 
PyDict_Next() which is part of the stable ABI).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Raymond Hettinger]

Raymond Hettinger  added the comment:

The likely culprit is the set_next() loop.  Perhaps it is never safe to use 
set_next() because any lookup can callback to __eq__ which can mutate the set.

Since set_isdisjoint() method isn't a mutating method, that is the easiest 
place to start investigating.  Try disabling the exact set fast path to see if 
the issue persists.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Dennis Sweeney]

Dennis Sweeney  added the comment:

set1.isdisjoint(set2) also crashes

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46615] Use-after-free by mutating set during set operations

[Dennis Sweeney]

Change by Dennis Sweeney :


--
title: Segfault in set intersection (&) and difference (-) -> Use-after-free by 
mutating set during set operations

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com