bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#22526: 25.0.90; Crash starting gnus


From: Eli Zaretskii
Subject: bug#22526: 25.0.90; Crash starting gnus
Date: Sun, 14 Feb 2016 00:11:58 +0200

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Sat, 13 Feb 2016 22:35:57 +0100
> Cc: andrewjmoreton@gmail.com, 22526@debbugs.gnu.org
> 
> I think we need the DebPrint() trace of the problem to conclude.

I think the patch I propose below will help in that.

> 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;
> }

See below, I think the patch I propose handles that as well.

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

It means the region after the one we are trying to enlarge was not
reserved by us, and we should call mmap_alloc (which we do).  No?

What I'm worried about is something else: the code is written under
the assumption that *var is the base address of the allocation, which
is why we use *var + memInfo.RegionSize to get to the next region.
But if *var is not the base address, this is wrong, and we should use
memInfo.BaseAddress instead, I think.  WDYT?

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

The error is ERROR_INVALID_PARAMETER (87), as Andy just reported.

> Admittedly, if there were problems of this nature, they would probably have 
> been observed on other platforms.

I agree that it's strange we only see this now on a single machine.
But maybe Andy does have memory content he doesn't know about.

Anyway, here's the patch I propose.  Andy, please apply this and then
run Emacs under GDB again, so that the error messages will be seen.
The patch includes the previous one.

diff --git a/src/w32heap.c b/src/w32heap.c
index 00da86a..a05c7f2 100644
--- a/src/w32heap.c
+++ b/src/w32heap.c
@@ -652,15 +652,19 @@ mmap_alloc (void **var, size_t nbytes)
     {
       /* Now, commit pages for NBYTES.  */
       *var = VirtualAlloc (p, nbytes, MEM_COMMIT, PAGE_READWRITE);
+      if (*var == NULL)
+       p = *var;
     }
 
   if (!p)
     {
-      if (GetLastError () == ERROR_NOT_ENOUGH_MEMORY)
+      DWORD e = GetLastError ();
+
+      if (e == ERROR_NOT_ENOUGH_MEMORY)
        errno = ENOMEM;
       else
        {
-         DebPrint (("mmap_alloc: error %ld\n", GetLastError ()));
+         DebPrint (("mmap_alloc: error %ld\n", e));
          errno = EINVAL;
        }
     }
@@ -700,6 +704,8 @@ mmap_realloc (void **var, size_t nbytes)
   /* We need to enlarge the block.  */
   if (memInfo.RegionSize < nbytes)
     {
+      void *old_ptr;
+
       if (VirtualQuery (*var + memInfo.RegionSize, &m2, sizeof(m2)) == 0)
         DebPrint (("mmap_realloc: VirtualQuery error = %ld\n",
                   GetLastError ()));
@@ -715,9 +721,11 @@ mmap_realloc (void **var, size_t nbytes)
                            MEM_COMMIT, PAGE_READWRITE);
          if (!p /* && GetLastError() != ERROR_NOT_ENOUGH_MEMORY */)
            {
-             DebPrint (("realloc enlarge: VirtualAlloc error %ld\n",
+             DebPrint (("realloc enlarge: VirtualAlloc (%p + %I64x, %I64x) 
error %ld\n",
+                        *var, (uint64_t)memInfo.RegionSize,
+                        (uint64_t)(nbytes - memInfo.RegionSize),
                         GetLastError ()));
-             errno = ENOMEM;
+             goto enlarge_block;
            }
          return *var;
        }
@@ -726,7 +734,8 @@ mmap_realloc (void **var, size_t nbytes)
          /* Else we must actually enlarge the block by allocating a
             new one and copying previous contents from the old to the
             new one.  */
-         void *old_ptr = *var;
+       enlarge_block:
+         old_ptr = *var;
 
          if (mmap_alloc (var, nbytes))
            {





reply via email to

[Prev in Thread] Current Thread [Next in Thread]