This is an archive of the discontinued Mercurial Phabricator instance.

revlog: check if PyInt_AS_LONG failed before using result
AbandonedPublic

Authored by durin42 on Nov 5 2018, 6:06 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Summary

This fixes

SystemError: PyEval_EvalFrameEx returned a result with an error set

in test-storage.py on Python 3. I believe we should audit *all*
callsites of PyInt_AS_LONG and related macros to hunt for similar
bugs. :(

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Nov 5 2018, 6:06 PM
indygreg requested changes to this revision.Nov 5 2018, 11:16 PM
indygreg added a subscriber: indygreg.

IIRC we #define PyInt and PyLong in the source for Python 3 compatibility. I remember doing something really hacky to unblock getting the C extensions to compile on Python 3 at the Paris sprint. It would definitely be a good idea to audit everything with PyInt and PyLong in it for sanity.

In this case, I suspect something really weird because PyInt_AS_LONG() doesn't exist on Python 3 and the docs for Python 2 say it performs no error checking. So the fact that it is setting an exception on Python 3 causes me to raise an eyebrow. Would you mind spending an extra few minutes investigating what's actually going on and explain it in the commit message?

This revision now requires changes to proceed.Nov 5 2018, 11:16 PM
yuja added a subscriber: yuja.Nov 6 2018, 7:38 AM
In this case, I suspect something really weird because `PyInt_AS_LONG()` doesn't exist on Python 3 and the docs for Python 2 say it performs no error checking. So the fact that it is setting an exception on Python 3 causes me to raise an eyebrow.

It's #define PyLong_AS_LONG(op) PyLong_AsLong(op), sigh. And the reason
of the lack of PyLong_AS_LONG(op) would be that Python 3 has no bounded
integer type.

+	if (PyInt_Check(value)) {
+		long arg = PyInt_AS_LONG(value);

In this case, we'll probably have to replace PyInt_Check() + PyInt_AS_LONG()
with PyInt_AsLong() + PyErr_Occurred().

In D5224#78093, @yuja wrote:
In this case, I suspect something really weird because `PyInt_AS_LONG()` doesn't exist on Python 3 and the docs for Python 2 say it performs no error checking. So the fact that it is setting an exception on Python 3 causes me to raise an eyebrow.

It's #define PyLong_AS_LONG(op) PyLong_AsLong(op), sigh. And the reason
of the lack of PyLong_AS_LONG(op) would be that Python 3 has no bounded
integer type.

+	if (PyInt_Check(value)) {
+		long arg = PyInt_AS_LONG(value);

In this case, we'll probably have to replace PyInt_Check() + PyInt_AS_LONG()
with PyInt_AsLong() + PyErr_Occurred().

Ugh, good catch. How about this:

https://phab.mercurial-scm.org/D5235

I'm not super in love with it, but it feels somewhat better encapsulated this way. Seems to work right on both versions...

durin42 abandoned this revision.Nov 13 2018, 2:02 PM