Patchwork [06,of,10,V2] revlog: add a native implementation of issnapshot

login
register
mail settings
Submitter Boris Feld
Date Dec. 21, 2018, 11:47 a.m.
Message ID <d15f3f46ca1e52f06d5b.1545392829@localhost.localdomain>
Download mbox | patch
Permalink /patch/37298/
State Accepted
Headers show

Comments

Boris Feld - Dec. 21, 2018, 11:47 a.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1545040633 -3600
#      Mon Dec 17 10:57:13 2018 +0100
# Node ID d15f3f46ca1e52f06d5b8f3f77c5e2e02912aaa9
# Parent  4067f496c37ec6d10860504186917bb6be83db46
# EXP-Topic sparse-revlog
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d15f3f46ca1e
revlog: add a native implementation of issnapshot

This will be used in the next changesets
Yuya Nishihara - Dec. 22, 2018, 4:07 a.m.
On Fri, 21 Dec 2018 12:47:09 +0100, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1545040633 -3600
> #      Mon Dec 17 10:57:13 2018 +0100
> # Node ID d15f3f46ca1e52f06d5b8f3f77c5e2e02912aaa9
> # Parent  4067f496c37ec6d10860504186917bb6be83db46
> # EXP-Topic sparse-revlog
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d15f3f46ca1e
> revlog: add a native implementation of issnapshot

> +static int index_issnapshotrev(indexObject *self, Py_ssize_t rev)
> +{
> +	int ps[2];
> +	Py_ssize_t base;
> +	if (rev == -1) {
> +		return 1;
> +	}
> +	base = (Py_ssize_t)index_baserev(self, rev);

Here base isn't guaranteed to be a valid revision, and passing it in to
index_issnapshotrev() will allow arbitrary memory read.

> +	if (base == rev) {
> +		base = -1;
> +	}
> +	if (base == -2) {
> +		assert(PyErr_Occurred());
> +		return -1;
> +	}
> +	if (base == -1) {
> +		return 1;
> +	}
> +	if (index_get_parents(self, rev, ps, (int)rev) < 0) {
> +		assert(PyErr_Occurred());
> +		return -1;
> +	};
> +	if (base == ps[0] || base == ps[1]) {
> +		return 0;
> +	}
> +	return index_issnapshotrev(self, base);

Maybe you missed the comment in the previous series, but this function will
never stop if base goes e.g. upwards at some point, and SEGV. Appears that
gcc can turn the recursion into loop in this case, but still there's a risk
of infinite loop.
Yuya Nishihara - Dec. 22, 2018, 4:38 a.m.
On Sat, 22 Dec 2018 13:07:25 +0900, Yuya Nishihara wrote:
> On Fri, 21 Dec 2018 12:47:09 +0100, Boris Feld wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld@octobus.net>
> > # Date 1545040633 -3600
> > #      Mon Dec 17 10:57:13 2018 +0100
> > # Node ID d15f3f46ca1e52f06d5b8f3f77c5e2e02912aaa9
> > # Parent  4067f496c37ec6d10860504186917bb6be83db46
> > # EXP-Topic sparse-revlog
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d15f3f46ca1e
> > revlog: add a native implementation of issnapshot

> > +	return index_issnapshotrev(self, base);
> 
> Maybe you missed the comment in the previous series, but this function will
> never stop if base goes e.g. upwards at some point, and SEGV. Appears that
> gcc can turn the recursion into loop in this case, but still there's a risk
> of infinite loop.

To be clear, I mean

 a. the recursion should be rewritten as a simple loop
 b. and the loop needs upper limit (e.g. i < rev + 1) to make sure it
    terminates
Boris Feld - Dec. 27, 2018, 11:26 p.m.
On 22/12/2018 05:38, Yuya Nishihara wrote:
> On Sat, 22 Dec 2018 13:07:25 +0900, Yuya Nishihara wrote:
>> On Fri, 21 Dec 2018 12:47:09 +0100, Boris Feld wrote:
>>> # HG changeset patch
>>> # User Boris Feld <boris.feld@octobus.net>
>>> # Date 1545040633 -3600
>>> #      Mon Dec 17 10:57:13 2018 +0100
>>> # Node ID d15f3f46ca1e52f06d5b8f3f77c5e2e02912aaa9
>>> # Parent  4067f496c37ec6d10860504186917bb6be83db46
>>> # EXP-Topic sparse-revlog
>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d15f3f46ca1e
>>> revlog: add a native implementation of issnapshot
>>> +	return index_issnapshotrev(self, base);
>> Maybe you missed the comment in the previous series, but this function will
>> never stop if base goes e.g. upwards at some point, and SEGV. Appears that
>> gcc can turn the recursion into loop in this case, but still there's a risk
>> of infinite loop.
> To be clear, I mean
>
>  a. the recursion should be rewritten as a simple loop
>  b. and the loop needs upper limit (e.g. i < rev + 1) to make sure it
>     terminates
I feel like any method using `index_baserev` could be affected (eg:
index_deltachain). I am updating the code to have `index_baserev` return
an error if it goes upward.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 28, 2018, 5:21 a.m.
On Fri, 28 Dec 2018 00:26:54 +0100, Boris FELD wrote:
> On 22/12/2018 05:38, Yuya Nishihara wrote:
> > On Sat, 22 Dec 2018 13:07:25 +0900, Yuya Nishihara wrote:
> >> On Fri, 21 Dec 2018 12:47:09 +0100, Boris Feld wrote:
> >>> # HG changeset patch
> >>> # User Boris Feld <boris.feld@octobus.net>
> >>> # Date 1545040633 -3600
> >>> #      Mon Dec 17 10:57:13 2018 +0100
> >>> # Node ID d15f3f46ca1e52f06d5b8f3f77c5e2e02912aaa9
> >>> # Parent  4067f496c37ec6d10860504186917bb6be83db46
> >>> # EXP-Topic sparse-revlog
> >>> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d15f3f46ca1e
> >>> revlog: add a native implementation of issnapshot
> >>> +	return index_issnapshotrev(self, base);
> >> Maybe you missed the comment in the previous series, but this function will
> >> never stop if base goes e.g. upwards at some point, and SEGV. Appears that
> >> gcc can turn the recursion into loop in this case, but still there's a risk
> >> of infinite loop.
> > To be clear, I mean
> >
> >  a. the recursion should be rewritten as a simple loop
> >  b. and the loop needs upper limit (e.g. i < rev + 1) to make sure it
> >     terminates
> I feel like any method using `index_baserev` could be affected (eg:
> index_deltachain). I am updating the code to have `index_baserev` return
> an error if it goes upward.

Good catch.

Please also rewrite the recursion as a loop for clarity. I don't think the
stack would be fully consumed in practice, but that's technically possible
with extremely long chain.

Patch

diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -986,6 +986,34 @@  static inline int index_baserev(indexObj
 	}
 }
 
+static int index_issnapshotrev(indexObject *self, Py_ssize_t rev)
+{
+	int ps[2];
+	Py_ssize_t base;
+	if (rev == -1) {
+		return 1;
+	}
+	base = (Py_ssize_t)index_baserev(self, rev);
+	if (base == rev) {
+		base = -1;
+	}
+	if (base == -2) {
+		assert(PyErr_Occurred());
+		return -1;
+	}
+	if (base == -1) {
+		return 1;
+	}
+	if (index_get_parents(self, rev, ps, (int)rev) < 0) {
+		assert(PyErr_Occurred());
+		return -1;
+	};
+	if (base == ps[0] || base == ps[1]) {
+		return 0;
+	}
+	return index_issnapshotrev(self, base);
+}
+
 static PyObject *index_deltachain(indexObject *self, PyObject *args)
 {
 	int rev, generaldelta;