From: Eli Zaretskii <eliz@gnu.org>
To: Fabrice Popineau <fabrice.popineau@gmail.com>
Cc: 22526@debbugs.gnu.org, andrewjmoreton@gmail.com
Subject: bug#22526: 25.0.90; Crash starting gnus
Date: Sun, 14 Feb 2016 00:11:58 +0200 [thread overview]
Message-ID: <83r3ggz2dt.fsf@gnu.org> (raw)
In-Reply-To: <CAFgFV9OQQZYqHaes5=Nh26sGQ8O3PPGqbgMW=SsVaMc0Fj1pZA@mail.gmail.com> (message from Fabrice Popineau on Sat, 13 Feb 2016 22:35:57 +0100)
> 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))
{
next prev parent reply other threads:[~2016-02-13 22:11 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-01 22:13 bug#22526: 25.0.90; Crash starting gnus Andy Moreton
2016-02-07 5:42 ` Lars Ingebrigtsen
2016-02-07 16:00 ` Eli Zaretskii
2016-02-07 20:58 ` Andy Moreton
2016-02-07 21:05 ` Eli Zaretskii
2016-02-11 2:06 ` Andy Moreton
2016-02-11 20:27 ` Eli Zaretskii
2016-02-11 21:20 ` Andy Moreton
2016-02-11 21:26 ` Eli Zaretskii
2016-02-12 13:34 ` Andy Moreton
2016-02-12 16:16 ` Eli Zaretskii
2016-02-12 22:26 ` Andy Moreton
2016-02-13 8:28 ` Eli Zaretskii
2016-02-13 10:44 ` Eli Zaretskii
2016-02-13 16:08 ` Fabrice Popineau
2016-02-13 16:42 ` Eli Zaretskii
2016-02-13 21:35 ` Fabrice Popineau
2016-02-13 22:11 ` Eli Zaretskii [this message]
2016-02-13 23:44 ` Fabrice Popineau
2016-02-14 5:49 ` Eli Zaretskii
2016-02-14 9:05 ` Fabrice Popineau
2016-02-14 16:57 ` Eli Zaretskii
2016-02-14 5:41 ` Eli Zaretskii
2016-02-14 14:17 ` Andy Moreton
2016-02-14 16:55 ` Eli Zaretskii
2016-02-14 17:51 ` Eli Zaretskii
2016-02-14 21:04 ` Fabrice Popineau
2016-02-14 21:29 ` Eli Zaretskii
2016-02-14 21:31 ` Fabrice Popineau
2016-02-14 21:34 ` Eli Zaretskii
2016-02-14 21:41 ` Fabrice Popineau
2016-02-15 3:32 ` Eli Zaretskii
2016-02-15 8:09 ` Fabrice Popineau
2016-02-15 11:39 ` Eli Zaretskii
2016-02-13 15:16 ` Andy Moreton
2016-02-13 15:52 ` Eli Zaretskii
2016-02-13 21:26 ` Andy Moreton
2016-02-16 1:18 ` Andy Moreton
2016-02-16 3:46 ` Eli Zaretskii
2016-02-20 11:08 ` Eli Zaretskii
2016-02-20 16:17 ` Andy Moreton
2016-02-20 17:01 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83r3ggz2dt.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=22526@debbugs.gnu.org \
--cc=andrewjmoreton@gmail.com \
--cc=fabrice.popineau@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.