D5224: revlog: check if PyInt_AS_LONG failed before using result
durin42 added a comment.
In https://phab.mercurial-scm.org/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...
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D5224
To: durin42, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5224: revlog: check if PyInt_AS_LONG failed before using result
yuja added a comment.
> 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()`.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D5224
To: durin42, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D5224: revlog: check if PyInt_AS_LONG failed before using result
> 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()`.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5224: revlog: check if PyInt_AS_LONG failed before using result
indygreg requested changes to this revision. indygreg added a comment. This revision now requires changes to proceed. 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? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5224 To: durin42, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5224: revlog: check if PyInt_AS_LONG failed before using result
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION 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. :(
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D5224
AFFECTED FILES
mercurial/cext/revlog.c
CHANGE DETAILS
diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -1377,8 +1377,13 @@
char *node;
int rev;
- if (PyInt_Check(value))
- return index_get(self, PyInt_AS_LONG(value));
+ if (PyInt_Check(value)) {
+ long arg = PyInt_AS_LONG(value);
+ if (PyErr_Occurred()) {
+ return NULL;
+ }
+ return index_get(self, arg);
+ }
if (node_check(value, ) == -1)
return NULL;
To: durin42, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
