From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Fabrice Popineau Newsgroups: gmane.emacs.bugs Subject: bug#22526: 25.0.90; Crash starting gnus Date: Sat, 13 Feb 2016 22:35:57 +0100 Message-ID: References: <56AFD88B.5040904@gmail.com> <87pow9cc0c.fsf@gnus.org> <83h9hkse78.fsf@gnu.org> <864mdk44q6.fsf@gmail.com> <83mvrcqli1.fsf@gnu.org> <86twlg2e69.fsf@gmail.com> <8360xv9ems.fsf@gnu.org> <8637sz7xmh.fsf@gmail.com> <83io1v7xcd.fsf@gnu.org> <83fuwx7vkv.fsf@gnu.org> <86fuwxk1l1.fsf@gmail.com> <837fi96mkq.fsf@gnu.org> <83vb5s6gas.fsf@gnu.org> <83d1s05zov.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=089e0160b7c6ded1ec052bad9289 X-Trace: ger.gmane.org 1455399441 11145 80.91.229.3 (13 Feb 2016 21:37:21 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 13 Feb 2016 21:37:21 +0000 (UTC) Cc: 22526@debbugs.gnu.org, andrewjmoreton@gmail.com To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat Feb 13 22:37:11 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aUhsN-0005pr-34 for geb-bug-gnu-emacs@m.gmane.org; Sat, 13 Feb 2016 22:37:11 +0100 Original-Received: from localhost ([::1]:45089 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUhsM-0005yh-BH for geb-bug-gnu-emacs@m.gmane.org; Sat, 13 Feb 2016 16:37:10 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:46879) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUhsH-0005yS-Ph for bug-gnu-emacs@gnu.org; Sat, 13 Feb 2016 16:37:07 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aUhsE-0002KQ-Is for bug-gnu-emacs@gnu.org; Sat, 13 Feb 2016 16:37:05 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:47606) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUhsE-0002KM-F0 for bug-gnu-emacs@gnu.org; Sat, 13 Feb 2016 16:37:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84) (envelope-from ) id 1aUhsE-0005e1-Aj for bug-gnu-emacs@gnu.org; Sat, 13 Feb 2016 16:37:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Fabrice Popineau Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 13 Feb 2016 21:37:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 22526 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 22526-submit@debbugs.gnu.org id=B22526.145539938421649 (code B ref 22526); Sat, 13 Feb 2016 21:37:02 +0000 Original-Received: (at 22526) by debbugs.gnu.org; 13 Feb 2016 21:36:24 +0000 Original-Received: from localhost ([127.0.0.1]:38381 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aUhrc-0005d6-8w for submit@debbugs.gnu.org; Sat, 13 Feb 2016 16:36:24 -0500 Original-Received: from mail-ob0-f171.google.com ([209.85.214.171]:35286) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aUhra-0005cu-DQ for 22526@debbugs.gnu.org; Sat, 13 Feb 2016 16:36:22 -0500 Original-Received: by mail-ob0-f171.google.com with SMTP id xk3so166356924obc.2 for <22526@debbugs.gnu.org>; Sat, 13 Feb 2016 13:36:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=QwApD4hTIymePFC3iT6ETiyr1YOVeG7RoQmQWEmMSr8=; b=uztgc4JUcWLPg+OmtfD1B61F/sC0Bbja4kYFSQpoyXSkDTOgX/SZQyi3YZgmfHIVe/ QbuVe9yOGBk5ZxoSfOcpi8jDELIvixlAdI3raCQ2Nx/nvGqExTOmFADfKDbI0RSufIR9 8qw/J21MKWrEzWHkZCQlPT+EA5yYKmpmaexystUST5EknaHs/yS9oFn+8LIj2xgkGgnO 8d4WQazNUxxykFgvNMcJfMkNB5iOgg+oT26uZjh/gzyVMVNQWETOUQQTuxapDS66heYv h5Jkalonn3caqMpRb+3D7p9DowJEWFGV8r8wo87bO0Ndgs+G44BSYlDUpCa07Og6ECj1 Czpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=QwApD4hTIymePFC3iT6ETiyr1YOVeG7RoQmQWEmMSr8=; b=DCAe85RqXkmStjb/M84j0gosA2739Joh+mxoo9KtCWjKXWl1LuOxlBs4Fq9pHR2EpC v2I7yGT1LHmvIrs0upDIsVXz3hLVpd0uWatOF8ZmnC8bXAfSamhaonqv4wx9qoIhQ9N/ 929Qj9Y7xLKYgcnRBLMCjwAFxQ3wq1RD6TJx+y8hosyuJ7b50mzmtB3QohAuBGHeJNRv 2TDcN5byB0OEbSBGFVDkvEv9EYOC6imQe3jRzUc4npI8fdgmuNw5n70cgwjN2CzgqFMj cLlmbtx2dbNhdcjXH6JNNY0vDnWV0fJ1tKFwPsHVDICQ/0y+xVOPnvMtWWGLXXFymtI/ /dLQ== X-Gm-Message-State: AG10YORLziuBePBMjO2pAbMKLT5RtkvGnQkXPk8gwIjaJabWyvQhnNhLd3V7C/1R8PHe+y7lTMKwRzwtiznWvQ== X-Received: by 10.182.73.225 with SMTP id o1mr7079460obv.80.1455399376964; Sat, 13 Feb 2016 13:36:16 -0800 (PST) Original-Received: by 10.202.78.131 with HTTP; Sat, 13 Feb 2016 13:35:57 -0800 (PST) In-Reply-To: <83d1s05zov.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:112996 Archived-At: --089e0160b7c6ded1ec052bad9289 Content-Type: text/plain; charset=UTF-8 2016-02-13 17:42 GMT+01:00 Eli Zaretskii : > > From: Fabrice Popineau > > Date: Sat, 13 Feb 2016 17:08:07 +0100 > > Cc: andrewjmoreton@gmail.com, 22526@debbugs.gnu.org > > > > Sorry for the delay with my answer, I'm trying to catch up with this > problem. > > No need to apologize. Thanks for chiming in. > > > First, and about the patch Eli has offered for mmap_realloc(), I would > be interested in knowing > > what was the error code at line 718: > > DebPrint (("realloc enlarge: VirtualAlloc error %ld\n", > > GetLastError ())); > > I don't think we know that, because I think Andy attached the debugger > only after the crash. But I sure hope to be wrong ;-) > > > I wonder if there is a case where it would fail on the VirtualAlloc() > and manage with the mmap_alloc() later. > > I agree than in the case of a failure with VirtualAlloc(), we don't > return NULL here, which may be the root > > of further problems. > > Yes. So you agree it's a good idea to commit that patch? > > I think we need the DebPrint() trace of the problem to conclude. If Andy could recompile with: diff --git a/src/w32fns.c b/src/w32fns.c index a5018ae..f439e36 100644 --- a/src/w32fns.c +++ b/src/w32fns.c @@ -8553,7 +8553,7 @@ _DebPrint (const char *fmt, ...) va_start (args, fmt); vsprintf (buf, fmt, args); va_end (args); -#if CYGWIN +#if 1 /* CYGWIN */ fprintf (stderr, "%s", buf); #endif OutputDebugString (buf); It would ease the printing/copying/pasting of DebPrint() messages. About w32heap.c, the very minimum that we need is : diff --git a/src/w32heap.c b/src/w32heap.c index 00da86a..91167cd 100644 --- a/src/w32heap.c +++ b/src/w32heap.c @@ -654,7 +654,7 @@ mmap_alloc (void **var, size_t nbytes) *var = VirtualAlloc (p, nbytes, MEM_COMMIT, PAGE_READWRITE); } - if (!p) + if (p == NULL || *var == NULL) { if (GetLastError () == ERROR_NOT_ENOUGH_MEMORY) errno = ENOMEM; @@ -718,6 +718,7 @@ mmap_realloc (void **var, size_t nbytes) DebPrint (("realloc enlarge: VirtualAlloc error %ld\n", GetLastError ())); errno = ENOMEM; + return NULL; } return *var; } About mmap_realloc(), I'm not sure a second attempt at reallocating the buffer at a different address has a better chance to succeed (but who knows ? Maybe you are right to try, but I would avoid the jump between the branches of the conditional). Anyway, there may be some other interference : /* If there is enough room in the current reserved area, then commit more pages as needed. */ if (m2.State == MEM_RESERVE && nbytes <= memInfo.RegionSize + m2.RegionSize) { If the address sent to mmap_realloc() has been messed up by another part of the code, we won't know it, VirtualQuery will return a wrong value and we will keep taking wrong decisions. For example, if m2.State is not MEM_RESERVE, what does that mean? Every block that we try to reallocate should have been allocated first, so reserved first. Are there other cases that could happen? The error codes from VirtualAlloc() here are crucial. Admittedly, if there were problems of this nature, they would probably have been observed on other platforms. > > Second, I don't see the problem in mmap_alloc(): if VirtualAlloc() > fails, p is NULL and this is the value returned > > at line 668: > > > > return *var = p; > > > > Am I missing something here ? > > I thought about the scenario where VirtualAlloc succeeds in the call > with MEM_RESERVE, but fails in the call with MEM_COMMIT. > Ok, agreed. > Please also read the rest of the thread, perhaps my conclusion about > mmap_realloc being the culprit as incorrect. I just don't see how > else to explain the fact that Emacs asked to enlarge the buffer beyond > 64KB, but got a valid pointer to only a 64KB memory region. > I'm positive on the fact that mmap_realloc() should have returned NULL, so that any caller could take the right action. At the moment, it failed to enlarge the bloc, but acted as if it were able to do so by returning its address. Hence cascading problems. Fabrice --089e0160b7c6ded1ec052bad9289 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


2016-02-13 17:42 GMT+01:00 Eli Zaretskii <eliz@gnu.org>:
=
> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Sat, 13 Feb 2016 17:08:07 +0100
> Cc: andrewjmoreton@gmail.c= om, 22526@debbugs.gnu.org<= br> >
> Sorry for the delay with my answer, I'm trying to catch up with th= is problem.

No need to apologize.=C2=A0 Thanks for chiming in.

> First, and about the patch Eli has offered for mmap_realloc(), I would= be interested in knowing
> what was the error code at line 718:
> DebPrint (("realloc enlarge: VirtualAlloc error %ld\n",
> GetLastError ()));

I don't think we know that, because I think Andy attached the de= bugger
only after the crash.=C2=A0 But I sure hope to be wrong ;-)

> I wonder if there is a case where it would fail on the VirtualAlloc() = and manage with the mmap_alloc() later.
> I agree than in the case of a failure with VirtualAlloc(), we don'= t return NULL here, which may be the root
> of further problems.

Yes.=C2=A0 So you agree it's a good idea to commit that patch?


I think we need the DebPrint() trace of the problem to conclude.
If Andy could recompile with:
diff --git a/src/w32fns.c b/s= rc/w32fns.c
index a5018ae..f439e36 100644
--- a/src/w32= fns.c
+++ b/src/w32fns.c
@@ -8553,7 +8553,7 @@ _DebPrin= t (const char *fmt, ...)
=C2=A0 =C2=A0va_start (args, fmt);
=
=C2=A0 =C2=A0vsprintf (buf, fmt, args);
=C2=A0 =C2=A0va_end = (args);
-#if CYGWIN
+#if 1 /* CYGWIN */
=C2= =A0 =C2=A0fprintf (stderr, "%s", buf);
=C2=A0#endif
=C2=A0 =C2=A0OutputDebugString (buf);

It would ease the printing/copying/pasting of DebPrint() messages.


About w32heap.c, the very minimum that we = need is :

diff --git a/src/w32heap.c b/src/w32heap= .c
index 00da86a..91167cd 100644
--- a/src/w32heap.c
+++ b/src/w32heap.c
@@ -654,7 +654,7 @@ mmap_alloc (void = **var, size_t nbytes)
=C2=A0 =C2=A0 =C2=A0 =C2=A0*var =3D Virtual= Alloc (p, nbytes, MEM_COMMIT, PAGE_READWRITE);
=C2=A0 =C2=A0 =C2= =A0}

- =C2=A0if (!p)
+ =C2=A0if (p =3D= =3D NULL || *var =3D=3D NULL)
=C2=A0 =C2=A0 =C2=A0{
=C2= =A0 =C2=A0 =C2=A0 =C2=A0if (GetLastError () =3D=3D ERROR_NOT_ENOUGH_MEMORY)=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 errno =3D ENOMEM;
@@ -718,= 6 +718,7 @@ mmap_realloc (void **var, size_t nbytes)
=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DebPrint (("realloc enlar= ge: VirtualAlloc error %ld\n",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 GetLastError= ()));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0err= no =3D ENOMEM;
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= return NULL;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return *var;
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 }

About mmap_realloc(), I= 'm not sure a second attempt at reallocating the buffer at a different = address has a better chance to succeed=C2=A0
(but who knows ? May= be you are right to try, but I would avoid the jump between the branches of= the conditional).

Anyway, there may be some other= interference :

=C2=A0 =C2=A0 =C2=A0 /* If there i= s enough room in the current reserved area, then
=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0commit more pages as needed. =C2=A0*/
=C2=A0 = =C2=A0 =C2=A0 if (m2.State =3D=3D MEM_RESERVE
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 && nbytes <=3D memInfo.RegionSize + m2.RegionS= ize)
{

If the address sent to mmap_realloc() has been messed up = by another part of the code, we won't know it, VirtualQuery will return= =C2=A0
a wrong value and we will keep taking wrong decisions. For= example, if m2.State is not MEM_RESERVE, what does that mean?
Ev= ery block that we try to reallocate should have been allocated first, so re= served first. Are there =C2=A0other cases that could happen?

=
The error codes from VirtualAlloc() here are crucial.
=
Admittedly, if there were problems of this nature, they woul= d probably have been observed on other platforms.
=C2=A0
> Second, I don't see the problem in mmap_alloc(): if VirtualAlloc()= fails, p is NULL and this is the value returned
> at line 668:
>
> return *var =3D p;
>
> Am I missing something here ?

I thought about the scenario where VirtualAlloc succeeds in the call=
with MEM_RESERVE, but fails in the call with MEM_COMMIT.

Ok, agreed.
=C2=A0
Please also read the rest of the thread, perhaps my conclusion about
mmap_realloc being the culprit as incorrect.=C2=A0 I just don't see how=
else to explain the fact that Emacs asked to enlarge the buffer beyond
64KB, but got a valid pointer to only a 64KB memory region.

I'm positive on= the fact that mmap_realloc() should have returned NULL, so that=C2=A0
any caller could take the right action. At the = moment, it failed to enlarge the bloc,=C2=A0
but acted as if it were able to do so by returning its address. Hence cas= cading
problems.

Fabrice
--089e0160b7c6ded1ec052bad9289--