Submitter | Josef 'Jeff' Sipek |
---|---|
Date | June 13, 2018, 2:55 p.m. |
Message ID | <d591c80025ee7316b772.1528901712@meili> |
Download | mbox | patch |
Permalink | /patch/32119/ |
State | Accepted |
Headers | show |
Comments
> 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.
Patch
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++) { if (self->lines[i].from_malloc) { free(self->lines[i].start); }