bug#22526: 25.0.90; Crash starting gnus

[Fabrice Popineau <fabrice.popineau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4200 bytes --]

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.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

[-- Attachment #2: Type: text/html, Size: 6478 bytes --]