Patchwork [4,of,4,V2] sparse-revlog: protect C code against delta chain including nullrev

login
register
mail settings
Submitter Boris Feld
Date Dec. 15, 2018, 3:10 p.m.
Message ID <e7d33dc28696a1b2ee95.1544886656@localhost.localdomain>
Download mbox | patch
Permalink /patch/37188/
State Accepted
Headers show

Comments

Boris Feld - Dec. 15, 2018, 3:10 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1544804741 -3600
#      Fri Dec 14 17:25:41 2018 +0100
# Node ID e7d33dc28696a1b2ee951cb82fe2cd611037afc8
# Parent  36c68746763d6b93b00c19387e79bd9cb623da72
# EXP-Topic sparse-followup
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e7d33dc28696
sparse-revlog: protect C code against delta chain including nullrev

For unclear reasons, some repositories include nullrev (-1). Re-computing
delta for such repo remove nullrev from all chain, so some older versions have
been creating them.

This currently raise an IndexError with the new C code doing chain slicing as
it expect all item to be positive.

Both python and C code for reading delta chain preserve nullrev, and the Python
code for chain slicing handle the case fine. So we take the safe route and make
the new C code works fine in that case.
Yuya Nishihara - Dec. 16, 2018, 1:33 a.m.
On Sat, 15 Dec 2018 15:10:56 +0000, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1544804741 -3600
> #      Fri Dec 14 17:25:41 2018 +0100
> # Node ID e7d33dc28696a1b2ee951cb82fe2cd611037afc8
> # Parent  36c68746763d6b93b00c19387e79bd9cb623da72
> # EXP-Topic sparse-followup
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e7d33dc28696
> sparse-revlog: protect C code against delta chain including nullrev
> 
> For unclear reasons, some repositories include nullrev (-1). Re-computing
> delta for such repo remove nullrev from all chain, so some older versions have
> been creating them.
> 
> This currently raise an IndexError with the new C code doing chain slicing as
> it expect all item to be positive.
> 
> Both python and C code for reading delta chain preserve nullrev, and the Python
> code for chain slicing handle the case fine. So we take the safe route and make
> the new C code works fine in that case.
> 
> diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
> --- a/mercurial/cext/revlog.c
> +++ b/mercurial/cext/revlog.c
> @@ -1195,7 +1195,7 @@ static PyObject *index_slicechunktodensi
>  		if (revnum == -1 && PyErr_Occurred()) {
>  			goto bail;
>  		}
> -		if (revnum < 0 || revnum >= idxlen) {
> +		if (revnum < -1 || revnum >= idxlen) {

s/-1/nullrev/ in flight.

As a follow up, please add tests where nullrev is involved. We have bad
history of buffer overflow in C.

Patch

diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -1195,7 +1195,7 @@  static PyObject *index_slicechunktodensi
 		if (revnum == -1 && PyErr_Occurred()) {
 			goto bail;
 		}
-		if (revnum < 0 || revnum >= idxlen) {
+		if (revnum < -1 || revnum >= idxlen) {
 			PyErr_Format(PyExc_IndexError,
 			             "index out of range: %zd", revnum);
 			goto bail;