Patchwork [2,of,2] lazymanifest: don't crash when out of memory (issue5916)

# HG changeset patch
# User Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
# Date 1528900880 14400
#      Wed Jun 13 10:41:20 2018 -0400
# Branch stable
# Node ID d591c80025ee7316b77235b2d71c4b0f01c03123
# Parent  cbb47a946bc0e0346bfc9f9ba505f9475de43606
lazymanifest: don't crash when out of memory (issue5916)

self->lines can be NULL if we failed to allocate memory for it.

> On Jun 13, 2018, at 10:55, Josef 'Jeff' Sipek <jeffpc@josefsipek.net> wrote:
> 
> # HG changeset patch
> # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
> # Date 1528900880 14400
> #      Wed Jun 13 10:41:20 2018 -0400
> # Branch stable
> # Node ID d591c80025ee7316b77235b2d71c4b0f01c03123
> # Parent  cbb47a946bc0e0346bfc9f9ba505f9475de43606
> lazymanifest: don't crash when out of memory (issue5916)

queued for stable, thanks

On Wed, 13 Jun 2018 10:55:12 -0400, Josef 'Jeff' Sipek wrote:
> # HG changeset patch
> # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
> # Date 1528900880 14400
> #      Wed Jun 13 10:41:20 2018 -0400
> # Branch stable
> # Node ID d591c80025ee7316b77235b2d71c4b0f01c03123
> # Parent  cbb47a946bc0e0346bfc9f9ba505f9475de43606
> lazymanifest: don't crash when out of memory (issue5916)
> 
> self->lines can be NULL if we failed to allocate memory for it.
> 
> diff --git a/mercurial/cext/manifest.c b/mercurial/cext/manifest.c
> --- a/mercurial/cext/manifest.c
> +++ b/mercurial/cext/manifest.c
> @@ -185,7 +185,7 @@ static void lazymanifest_dealloc(lazyman
>  {
>  	/* free any extra lines we had to allocate */
>  	int i;
> -	for (i = 0; i < self->numlines; i++) {
> +	for (i = 0; self->lines && (i < self->numlines); i++) {

Perhaps realloc_if_full() shouldn't overwrite self->lines and maxlines on
failure. I think that's the source of the inconsistency.

On Thu, Jun 14, 2018 at 20:14:24 +0900, Yuya Nishihara wrote:
> On Wed, 13 Jun 2018 10:55:12 -0400, Josef 'Jeff' Sipek wrote:
> > # HG changeset patch
> > # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
> > # Date 1528900880 14400
> > #      Wed Jun 13 10:41:20 2018 -0400
> > # Branch stable
> > # Node ID d591c80025ee7316b77235b2d71c4b0f01c03123
> > # Parent  cbb47a946bc0e0346bfc9f9ba505f9475de43606
> > lazymanifest: don't crash when out of memory (issue5916)
> > 
> > self->lines can be NULL if we failed to allocate memory for it.
> > 
> > diff --git a/mercurial/cext/manifest.c b/mercurial/cext/manifest.c
> > --- a/mercurial/cext/manifest.c
> > +++ b/mercurial/cext/manifest.c
> > @@ -185,7 +185,7 @@ static void lazymanifest_dealloc(lazyman
> >  {
> >  	/* free any extra lines we had to allocate */
> >  	int i;
> > -	for (i = 0; i < self->numlines; i++) {
> > +	for (i = 0; self->lines && (i < self->numlines); i++) {
> 
> Perhaps realloc_if_full() shouldn't overwrite self->lines and maxlines on
> failure. I think that's the source of the inconsistency.

That is one possible place, but not the one I've hit in production.  I've
seen lazymanifest_copy() fail to allocate memory for ->lines.  I'm not
familiar with all the python goo, but I'm guessing:

1. lazymanifest_copy() is called
2. a new python object is allocated (copy)
3. copy->lines = malloc(...) = NULL because of ENOMEM
4. goto nomem
5. Py_XDECREF(copy)
6. lazymanifest_dealloc() is called by python goo

All in all, it looks like there are 4 places that allocate memory for
->lines.

Jeff.

On Thu, 14 Jun 2018 08:09:06 -0400, Josef 'Jeff' Sipek wrote:
> On Thu, Jun 14, 2018 at 20:14:24 +0900, Yuya Nishihara wrote:
> > On Wed, 13 Jun 2018 10:55:12 -0400, Josef 'Jeff' Sipek wrote:
> > > # HG changeset patch
> > > # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
> > > # Date 1528900880 14400
> > > #      Wed Jun 13 10:41:20 2018 -0400
> > > # Branch stable
> > > # Node ID d591c80025ee7316b77235b2d71c4b0f01c03123
> > > # Parent  cbb47a946bc0e0346bfc9f9ba505f9475de43606
> > > lazymanifest: don't crash when out of memory (issue5916)
> > > 
> > > self->lines can be NULL if we failed to allocate memory for it.
> > > 
> > > diff --git a/mercurial/cext/manifest.c b/mercurial/cext/manifest.c
> > > --- a/mercurial/cext/manifest.c
> > > +++ b/mercurial/cext/manifest.c
> > > @@ -185,7 +185,7 @@ static void lazymanifest_dealloc(lazyman
> > >  {
> > >  	/* free any extra lines we had to allocate */
> > >  	int i;
> > > -	for (i = 0; i < self->numlines; i++) {
> > > +	for (i = 0; self->lines && (i < self->numlines); i++) {
> > 
> > Perhaps realloc_if_full() shouldn't overwrite self->lines and maxlines on
> > failure. I think that's the source of the inconsistency.
> 
> That is one possible place, but not the one I've hit in production.  I've
> seen lazymanifest_copy() fail to allocate memory for ->lines.  I'm not
> familiar with all the python goo, but I'm guessing:
> 
> 1. lazymanifest_copy() is called
> 2. a new python object is allocated (copy)
> 3. copy->lines = malloc(...) = NULL because of ENOMEM
> 4. goto nomem
> 5. Py_XDECREF(copy)
> 6. lazymanifest_dealloc() is called by python goo
> 
> All in all, it looks like there are 4 places that allocate memory for
> ->lines.

Indeed. IIRC, tp_dealloc isn't paired with tp_init. I'll check if all
fields are initialized to sane values on nomem case.