Broken autoconf mmap test (was Re: 1.7] BUG - GREP slows to a crawl with large number of matches on a single file)

[Corinna Vinschen-2]

On Nov  8 14:56, Corinna Vinschen wrote:
> Btw., the check for mmap in grep's configure file is broken.  It tries
> to mmap to a fixed address formerly allocated via malloc().  This doesn't
> work on Windows.  An autoconf run with a newer version of autoconf would
> be nice.

I just found that the latest autoconf *still* has this broken test
for mmap, which basically calls

  data2 = malloc (size);
  mmap(data2, ...);

Why has this test never been fixed?  Chuck?


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Ralph Hempel]

Corinna Vinschen wrote:

> On Nov  8 14:56, Corinna Vinschen wrote:
>> Btw., the check for mmap in grep's configure file is broken.  It tries
>> to mmap to a fixed address formerly allocated via malloc().  This doesn't
>> work on Windows.  An autoconf run with a newer version of autoconf would
>> be nice.
>
> I just found that the latest autoconf *still* has this broken test
> for mmap, which basically calls
>
>   data2 = malloc (size);
>   mmap(data2, ...);
>
> Why has this test never been fixed?  Chuck?

I can't answer that question but this thread points out very important
lessons in debugging specifically and projects in general.

1. Easily reproducible test cases are critical to getting somone
    interested in fixing your problem.

2. Having the good fortune to have somebody run the test case and
    duplicate the problem helps a bit more.

3. Having that person challenge the assumptions under which the code
    has been working for YEARS without a complaint helps a bit more.

4. Having that person do a great analysis that shows why the problem
    exists helps even more.

5. Going even one step further and trying to figure out why the
    problem has existed for years and what else might be wrong is
    just the icing on the cake.

Bravo Corinna - on a Sunday no less...

Cheers, Ralph

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Christopher Faylor-8]

On Sun, Nov 08, 2009 at 10:51:56AM -0500, Ralph Hempel wrote:

>Corinna Vinschen wrote:
>> On Nov  8 14:56, Corinna Vinschen wrote:
>>> Btw., the check for mmap in grep's configure file is broken.  It tries
>>> to mmap to a fixed address formerly allocated via malloc().  This doesn't
>>> work on Windows.  An autoconf run with a newer version of autoconf would
>>> be nice.
>>
>> I just found that the latest autoconf *still* has this broken test
>> for mmap, which basically calls
>>
>>   data2 = malloc (size);
>>   mmap(data2, ...);
>>
>> Why has this test never been fixed?  Chuck?
>
>I can't answer that question but this thread points out very important
>lessons in debugging specifically and projects in general.
>
>1. Easily reproducible test cases are critical to getting somone
>    interested in fixing your problem.
>
>2. Having the good fortune to have somebody run the test case and
>    duplicate the problem helps a bit more.
>
>3. Having that person challenge the assumptions under which the code
>    has been working for YEARS without a complaint helps a bit more.
>
>4. Having that person do a great analysis that shows why the problem
>    exists helps even more.
>
>5. Going even one step further and trying to figure out why the
>    problem has existed for years and what else might be wrong is
>    just the icing on the cake.
>
>Bravo Corinna - on a Sunday no less...

6. googling for the problem is always a good thing to do.

Once it was clear that this was a character set issue in grep it was
easy enough to find a fix since it was already in a couple of linux bug
trackers.

cgf

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Charles Wilson-2]

Corinna Vinschen wrote:

> On Nov  8 14:56, Corinna Vinschen wrote:
>> Btw., the check for mmap in grep's configure file is broken.  It tries
>> to mmap to a fixed address formerly allocated via malloc().  This doesn't
>> work on Windows.  An autoconf run with a newer version of autoconf would
>> be nice.
>
> I just found that the latest autoconf *still* has this broken test
> for mmap, which basically calls
>
>   data2 = malloc (size);
>   mmap(data2, ...);
>
> Why has this test never been fixed?  Chuck?

...err, 'cause I didn't realize it was a problem. I see that cygport has
hidden this for years:

    # AC_HAVE_MMAP fails despite a working mmap, so we force this to yes
    # (see http://www.cygwin.com/ml/cygwin/2004-09/msg00741.html
    # and following thread for details)
    export ac_cv_func_mmap_fixed_mapped=yes;

NTTAWWT, but it never triggered my "gee I ought to fix that" reflex. I
agree this should be fixed, but I'm leery of changing an autoconf test
without knowing how that change will affect the other 9,236 platforms
that may depend on the current behavior, esp. given my current (lack of)
knowledge about how mmap is *supposed* to work in the various MAP_* modes.

I think this is an issue for the autoconf list as a whole.  Would you --
or Eric -- care to raise it there?  Especially as you seemed to have
quite strong feelings about it back in 2004:
http://www.cygwin.com/ml/cygwin/2004-09/msg00753.html
> The mmap test is crap.  How can an application expect to be able to
> access just about every address together with MAP_FIXED?
>
> Consequentially MapViewOfFileEx returns error 487 in these cases,
> "Attempt to access invalid address."
>
> That's just another example of a crappy autoconf mmap test.

--
Chuck

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Corinna Vinschen-2]

On Nov  8 14:07, Charles Wilson wrote:

> Corinna Vinschen wrote:
> > On Nov  8 14:56, Corinna Vinschen wrote:
> >> Btw., the check for mmap in grep's configure file is broken.  It tries
> >> to mmap to a fixed address formerly allocated via malloc().  This doesn't
> >> work on Windows.  An autoconf run with a newer version of autoconf would
> >> be nice.
> >
> > I just found that the latest autoconf *still* has this broken test
> > for mmap, which basically calls
> >
> >   data2 = malloc (size);
> >   mmap(data2, ...);
> >
> > Why has this test never been fixed?  Chuck?
>
> ...err, 'cause I didn't realize it was a problem. I see that cygport has
> hidden this for years:
>
>     # AC_HAVE_MMAP fails despite a working mmap, so we force this to yes
>     # (see http://www.cygwin.com/ml/cygwin/2004-09/msg00741.html
>     # and following thread for details)
>     export ac_cv_func_mmap_fixed_mapped=yes;
>
> NTTAWWT, but it never triggered my "gee I ought to fix that" reflex. I
> agree this should be fixed, but I'm leery of changing an autoconf test
> without knowing how that change will affect the other 9,236 platforms

The problem in this testcase is the fact that it calls malloc, then
computes the next page-aligned free address after the mallocated area
and then tries to mmap to this address with MAP_FIXED set.  Sure, this
*might* work, and it works on most systems, but there's no reason at all
to *expect* that it works since it only works by chance.  The memory
addresses can be taken by anything and to require that an arbitrary
fixed address is available to mmap is just plain wrong.  From the
Linux man page:

MAP_FIXED
  [...]
  If the specified address cannot be used, mmap() will fail.  Because
  requiring a fixed address for a mapping is less portable, the use of
  this option is discouraged.

Since autoconf is supposed to help applications to be more portable,
it's not really feasible, IMHO, that autoconf requires a non-portable
feature to work.

It's frustrating that mmap() and even mmap(MAP_FIXED)
works fine on Cygwin, just not in the non-portable way it's tested
in the autoconf test.  Maybe we need two mmap tests in autconf, one
for mmap in general, the other for MAP_FIXED iisues.

> I think this is an issue for the autoconf list as a whole.  Would you --
> or Eric -- care to raise it there?  Especially as you seemed to have
> quite strong feelings about it back in 2004:
> http://www.cygwin.com/ml/cygwin/2004-09/msg00753.html

I had hoped that you, as the autoconf maintainer, would put this
upstream...


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Corinna Vinschen on 11/9/2009 4:59 AM:

>>> I just found that the latest autoconf *still* has this broken test
>>> for mmap, which basically calls
>>>
>>>   data2 = malloc (size);
>>>   mmap(data2, ...);
>>>
>>> Why has this test never been fixed?  Chuck?
>> ...err, 'cause I didn't realize it was a problem. I see that cygport has
>> hidden this for years:
>>
>>     # AC_HAVE_MMAP fails despite a working mmap, so we force this to yes
>>     # (see http://www.cygwin.com/ml/cygwin/2004-09/msg00741.html
>>     # and following thread for details)
>>     export ac_cv_func_mmap_fixed_mapped=yes;
>>
>> NTTAWWT, but it never triggered my "gee I ought to fix that" reflex. I
>> agree this should be fixed, but I'm leery of changing an autoconf test
>> without knowing how that change will affect the other 9,236 platforms
>
> The problem in this testcase is the fact that it calls malloc, then
> computes the next page-aligned free address after the mallocated area
> and then tries to mmap to this address with MAP_FIXED set.  Sure, this
> *might* work, and it works on most systems, but there's no reason at all
> to *expect* that it works since it only works by chance.  The memory
> addresses can be taken by anything and to require that an arbitrary
> fixed address is available to mmap is just plain wrong.  From the
> Linux man page:
>
> MAP_FIXED
>   [...]
>   If the specified address cannot be used, mmap() will fail.  Because
>   requiring a fixed address for a mapping is less portable, the use of
>   this option is discouraged.
>
> Since autoconf is supposed to help applications to be more portable,
> it's not really feasible, IMHO, that autoconf requires a non-portable
> feature to work.
>
> It's frustrating that mmap() and even mmap(MAP_FIXED)
> works fine on Cygwin, just not in the non-portable way it's tested
> in the autoconf test.  Maybe we need two mmap tests in autconf, one
> for mmap in general, the other for MAP_FIXED iisues.
>
>> I think this is an issue for the autoconf list as a whole.  Would you --
>> or Eric -- care to raise it there?  Especially as you seemed to have
>> quite strong feelings about it back in 2004:
>> http://www.cygwin.com/ml/cygwin/2004-09/msg00753.html
>
> I had hoped that you, as the autoconf maintainer, would put this
> upstream...

It's an upstream issue now ;)

The problem is that I need some more advice from the cygwin list on how
best to fix the test to pass on cygwin by default.  I'm hoping to release
autoconf 2.65 this week, so a speedy fix to help this issue go away before
the release would be extra nice.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkr4D/sACgkQ84KuGfSFAYCOjwCghVcvxtUrAPxqB7w+/6gaT+Y/
H0EAoIUsDfqQ42NzKa8olQtBdhkvVS1f
=36fe
-----END PGP SIGNATURE-----

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Corinna Vinschen-2]

On Nov  9 05:50, Eric Blake wrote:

> According to Corinna Vinschen on 11/9/2009 4:59 AM:
> > MAP_FIXED
> >   [...]
> >   If the specified address cannot be used, mmap() will fail.  Because
> >   requiring a fixed address for a mapping is less portable, the use of
> >   this option is discouraged.
>
> It's an upstream issue now ;)
>
> The problem is that I need some more advice from the cygwin list on how
> best to fix the test to pass on cygwin by default.  I'm hoping to release
> autoconf 2.65 this week, so a speedy fix to help this issue go away before
> the release would be extra nice.

This part of the testcase

  data2 = (char *) malloc (2 * pagesize);
  if (!data2)
    return 1;
  data2 += (pagesize - ((long int) data2 & (pagesize - 1))) & (pagesize - 1);
  if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,
                       MAP_PRIVATE | MAP_FIXED, fd, 0L))
    return 1;

is bad.  The chance that the address of data2 is not usable for mmap on
Windows/Cygwin is 100%.  The problem here is that the generic HAVE_MMAP
test tests one certain feature, which is not usable on Windows, and which
is non-portable.  So, on Cygwin this test always fails and all applications
using this test in good faith will never use mmap on Cygwin, just because
the single case of "mmap private fixed at somewhere already mapped" doesn't
work.  In fact, most applications don't need this case.
And grep wouldn't need it either, since the method used in grep would
also work if the area hadn't been malloced before, if it would just use
the address returned by mmap as buffer.

That's why I think we need at least two tests in autoconf, a generic
mmap test and a mmap test for the "mmap private/shared fixed at
somewhere already mapped" case, if an application actually insists on
using that.


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Corinna Vinschen on 11/9/2009 7:05 AM:

> This part of the testcase
>
>   data2 = (char *) malloc (2 * pagesize);
>   if (!data2)
>     return 1;
>   data2 += (pagesize - ((long int) data2 & (pagesize - 1))) & (pagesize - 1);
>   if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,
>                        MAP_PRIVATE | MAP_FIXED, fd, 0L))
>     return 1;
>
> is bad.  The chance that the address of data2 is not usable for mmap on
> Windows/Cygwin is 100%.

But in testing this further, I discovered that you CAN do:

data2 = mmap(...);
munmap (data2,...);
mmap (data2, ... MAP_FIXED)

and get success on cygwin.  So I will be updating autoconf accordingly,
based on the STD below.  Unfortunately, it looks like I also found a hole
in cygwin.  Consider this (borrowing heavily from the autoconf test that I
am fixing):

#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/mman.h>

int
main (int argc, char **argv)
{
  char *data, *data2, *data3;
  int i, pagesize;
  int fd, fd2;

  pagesize = getpagesize ();
  /* First, make a file with some known garbage in it. */
  data = (char *) malloc (pagesize);
  if (!data)
    return 1;
  for (i = 0; i < pagesize; ++i)
    *(data + i) = rand ();
  umask (0);
  fd = creat ("conftest.mmap", 0600);
  if (fd < 0)
    return 2;
  if (write (fd, data, pagesize) != pagesize)
    return 3;
  close (fd);

  /* Next, check that a page is zero-filled if not backed by a file.  */
  fd2 = open ("conftest.txt", O_RDWR | O_CREAT | O_TRUNC, 0600);
  if (fd2 < 0)
    return 11;
  data2 = "";
  if (write (fd2, data2, 1) != 1)
    return 12;
  else
    /* We expect mmap to succeed, but reads to give SIGBUS, since mapped
       region is an entire page beyond bounds of mapped file.  */
    ;
  data2 = mmap (0, pagesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd2, 0L);
  if (data2 == MAP_FAILED)
    return 14;
  printf ("mapped %p\n", data2);
  for (i = 0; i < pagesize; ++i)
    if (*(data2 + i))
      {
        printf ("%p, %x\n", data2 + i, *(data2 + i));
        return 15;
      }
  close (fd2);
  if (argc > 1)
    munmap (data2, pagesize);

  /* Next, try to mmap the file at a fixed address which already has
     something else allocated at it.  If we can, also make sure that
     we see the same garbage.  */
  fd = open ("conftest.mmap", O_RDWR);
  if (fd < 0)
    return 4;
  if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,
                   MAP_PRIVATE | MAP_FIXED, fd, 0L))
    return 6;
  for (i = 0; i < pagesize; ++i)
    if (*(data + i) != *(data2 + i))
      {
        printf ("%p, exp %x, got %x\n", data2 + i, *(data + i), *(data2 + i));
        return 7;
      }

  /* Finally, make sure that changes to the mapped area do not
     percolate back to the file as seen by read().  (This is a bug on
     some variants of i386 svr4.0.)  */
  for (i = 0; i < pagesize; ++i)
    *(data2 + i) = *(data2 + i) + 1;
  data3 = (char *) malloc (pagesize);
  if (!data3)
    return 8;
  if (read (fd, data3, pagesize) != pagesize)
    return 9;
  for (i = 0; i < pagesize; ++i)
    if (*(data + i) != *(data3 + i))
      return 10;
  close (fd);
  return 0;
}

This test behaves differently on Linux than on cygwin; on Linux, both
'./foo' and './foo 1' give status 0, but on cygwin, './foo' gives status
6, and only './foo 1' succeeds.  In other words, the second mmap fails if
there is no intermediate munmap.

POSIX apparently allows cygwin's behavior:

"If MAP_FIXED is set, mmap() may return MAP_FAILED and set errno to
[EINVAL]. If a MAP_FIXED request is successful, the mapping established by
mmap() replaces any previous mappings for the pages in the range
[pa,pa+len) of the process."

However, since we already have to maintain a list of mappings in order to
implement fork(), it seems like it would be easy to fix cygwin to
implicitly munmap anything that would otherwise be in the way of a
subsequent MAP_FIXED request, rather than blindly calling
NtMapViewOfSection and failing because of the overlap, so that we could be
even more like Linux behavior.

> That's why I think we need at least two tests in autoconf, a generic
> mmap test and a mmap test for the "mmap private/shared fixed at
> somewhere already mapped" case, if an application actually insists on
> using that.

In the case of the autoconf test, I think a single test is still
sufficient, once it is fixed to be portable to what POSIX requires.

gnulib provides a more interesting test, for whether MMAP_ANON works.
http://git.savannah.gnu.org/cgit/gnulib.git/tree/m4/mmap-anon.m4

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkr46LMACgkQ84KuGfSFAYCBrwCgsu2/rWozZs/1R33RaAlUwHow
aLQAoNVjQ8P9it7nkDv8u2RRF4l0uDur
=D/jK
-----END PGP SIGNATURE-----

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Charles Wilson-2]

Corinna Vinschen wrote:
> I had hoped that you, as the autoconf maintainer, would put this
> upstream...

Well, I would have done so...but you guys beat me to it.  I go off-net
for a day or so, and lookit what happens...I think I need to go off-net
more often!

--
Chuck

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[please limit replies about the patch itself to autoconf-patches]

According to Corinna Vinschen on 11/9/2009 7:05 AM:

> This part of the testcase
>
>   data2 = (char *) malloc (2 * pagesize);
>   if (!data2)
>     return 1;
>   data2 += (pagesize - ((long int) data2 & (pagesize - 1))) & (pagesize - 1);
>   if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,
>                        MAP_PRIVATE | MAP_FIXED, fd, 0L))
>     return 1;
>
> is bad.  The chance that the address of data2 is not usable for mmap on
> Windows/Cygwin is 100%.  The problem here is that the generic HAVE_MMAP
> test tests one certain feature, which is not usable on Windows, and which
> is non-portable.

MAP_FIXED appears to be more portable when the fixed address was obtained
from a previous mmap call.  Therefore, this patch fixes the macro as well
as making diagnosing configure failures more accurately pinpoint why they
are declaring failure.  I don't have access to HP-UX 11, which is another
platform where AC_FUNC_MMAP was failing; I would appreciate if someone
else could see if this makes a difference there.  But I have verified that
this now sets HAVE_MMAP for cygwin 1.5.x and cygwin 1.7 where the old
version failed, and that it does not change behavior on Linux or OpenBSD.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkr484kACgkQ84KuGfSFAYDCIgCbBl/eHS9C9acPwXp5Krk7KAeF
zAIAoMBEbnQm5tLpRDkCFWhEXNieL5cf
=3fYB
-----END PGP SIGNATURE-----

From fb1f28a2ff2c688e63dc97ece7fde86e16864491 Mon Sep 17 00:00:00 2001
From: Eric Blake <ebb9@...>
Date: Mon, 9 Nov 2009 21:45:00 -0700
Subject: [PATCH] Fix AC_FUNC_MMAP for cygwin.

* lib/autoconf/functions.m4 (AC_FUNC_MMAP): Make the test more
portable: Actually check for <sys/param.h>, and only use MAP_FIXED
on an address previously returned from mmap.
* THANKS: Update.
Reported by Corinna Vinschen.

Signed-off-by: Eric Blake <ebb9@...>
---
 ChangeLog                 |    9 +++++++
 NEWS                      |    3 ++
 lib/autoconf/functions.m4 |   55 ++++++++++++++++++++++++++------------------
 3 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4d028c0..77e9d4e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
[phone]  Eric Blake  <ebb9@...>
+
+ Fix AC_FUNC_MMAP for cygwin.
+ * lib/autoconf/functions.m4 (AC_FUNC_MMAP): Make the test more
+ portable: Actually check for <sys/param.h>, and only use MAP_FIXED
+ on an address previously returned from mmap.
+ * THANKS: Update.
+ Reported by Corinna Vinschen.
+
 2009-11-04  Eric Blake  <ebb9@...>

  Redocument AS_DIRNAME, even with its flaws.
diff --git a/NEWS b/NEWS
index 9e7e64c..86a0c3f 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,9 @@ GNU Autoconf NEWS - User visible changes.
    longer mistakenly select a 32-bit type on some compilers (bug present
    since macros were introduced in 2.59c).

+** The AC_FUNC_MMAP macro has been fixed to be portable to systems like
+   Cygwin (bug present since macro was introduced in 2.0).
+
 ** The following documented autotest macros are new:
    AT_CHECK_EUNIT

diff --git a/lib/autoconf/functions.m4 b/lib/autoconf/functions.m4
index 946a646..6b6e7fc 100644
--- a/lib/autoconf/functions.m4
+++ b/lib/autoconf/functions.m4
@@ -1186,9 +1186,9 @@ AU_ALIAS([AM_FUNC_MKTIME], [AC_FUNC_MKTIME])
 # ------------
 AN_FUNCTION([mmap], [AC_FUNC_MMAP])
 AC_DEFUN([AC_FUNC_MMAP],
-[AC_CHECK_HEADERS(stdlib.h unistd.h)
-AC_CHECK_FUNCS(getpagesize)
-AC_CACHE_CHECK(for working mmap, ac_cv_func_mmap_fixed_mapped,
+[AC_CHECK_HEADERS_ONCE([stdlib.h unistd.h sys/param.h])
+AC_CHECK_FUNCS([getpagesize])
+AC_CACHE_CHECK([for working mmap], [ac_cv_func_mmap_fixed_mapped],
 [AC_RUN_IFELSE([AC_LANG_SOURCE([AC_INCLUDES_DEFAULT]
 [[/* malloc might have been renamed as rpl_malloc. */
 #undef malloc
@@ -1224,11 +1224,6 @@ char *malloc ();

 /* This mess was copied from the GNU getpagesize.h.  */
 #ifndef HAVE_GETPAGESIZE
-/* Assume that all systems that can run configure have sys/param.h.  */
-# ifndef HAVE_SYS_PARAM_H
-#  define HAVE_SYS_PARAM_H 1
-# endif
-
 # ifdef _SC_PAGESIZE
 #  define getpagesize() sysconf(_SC_PAGESIZE)
 # else /* no _SC_PAGESIZE */
@@ -1264,7 +1259,7 @@ main ()
 {
   char *data, *data2, *data3;
   int i, pagesize;
-  int fd;
+  int fd, fd2;

   pagesize = getpagesize ();

@@ -1277,27 +1272,41 @@ main ()
   umask (0);
   fd = creat ("conftest.mmap", 0600);
   if (fd < 0)
-    return 1;
+    return 2;
   if (write (fd, data, pagesize) != pagesize)
-    return 1;
+    return 3;
   close (fd);

+  /* Next, check that the tail of a page is zero-filled.  File must have
+     non-zero length, otherwise we risk SIGBUS for entire page.  */
+  fd2 = open ("conftest.txt", O_RDWR | O_CREAT | O_TRUNC, 0600);
+  if (fd2 < 0)
+    return 4;
+  data2 = "";
+  if (write (fd2, data2, 1) != 1)
+    return 5;
+  data2 = mmap (0, pagesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd2, 0L);
+  if (data2 == MAP_FAILED)
+    return 6;
+  for (i = 0; i < pagesize; ++i)
+    if (*(data2 + i))
+      return 7;
+  close (fd2);
+  if (munmap (data2, pagesize))
+    return 8;
+
   /* Next, try to mmap the file at a fixed address which already has
      something else allocated at it.  If we can, also make sure that
      we see the same garbage.  */
   fd = open ("conftest.mmap", O_RDWR);
   if (fd < 0)
-    return 1;
-  data2 = (char *) malloc (2 * pagesize);
-  if (!data2)
-    return 1;
-  data2 += (pagesize - ((long int) data2 & (pagesize - 1))) & (pagesize - 1);
+    return 9;
   if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,
      MAP_PRIVATE | MAP_FIXED, fd, 0L))
-    return 1;
+    return 10;
   for (i = 0; i < pagesize; ++i)
     if (*(data + i) != *(data2 + i))
-      return 1;
+      return 11;

   /* Finally, make sure that changes to the mapped area do not
      percolate back to the file as seen by read().  (This is a bug on
@@ -1306,12 +1315,12 @@ main ()
     *(data2 + i) = *(data2 + i) + 1;
   data3 = (char *) malloc (pagesize);
   if (!data3)
-    return 1;
+    return 12;
   if (read (fd, data3, pagesize) != pagesize)
-    return 1;
+    return 13;
   for (i = 0; i < pagesize; ++i)
     if (*(data + i) != *(data3 + i))
-      return 1;
+      return 14;
   close (fd);
   return 0;
 }]])],
@@ -1319,10 +1328,10 @@ main ()
        [ac_cv_func_mmap_fixed_mapped=no],
        [ac_cv_func_mmap_fixed_mapped=no])])
 if test $ac_cv_func_mmap_fixed_mapped = yes; then
-  AC_DEFINE(HAVE_MMAP, 1,
+  AC_DEFINE([HAVE_MMAP], [1],
     [Define to 1 if you have a working `mmap' system call.])
 fi
-rm -f conftest.mmap
+rm -f conftest.mmap conftest.txt
 ])# AC_FUNC_MMAP


--
1.6.5.rc1



--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

[Corinna Vinschen-2]

On Nov  9 21:14, Eric Blake wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> According to Corinna Vinschen on 11/9/2009 7:05 AM:
> > This part of the testcase
> >
> >   data2 = (char *) malloc (2 * pagesize);
> >   if (!data2)
> >     return 1;
> >   data2 += (pagesize - ((long int) data2 & (pagesize - 1))) & (pagesize - 1);
> >   if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,
> >                        MAP_PRIVATE | MAP_FIXED, fd, 0L))
> >     return 1;
> >
> > is bad.  The chance that the address of data2 is not usable for mmap on
> > Windows/Cygwin is 100%.
>
> But in testing this further, I discovered that you CAN do:
>
> data2 = mmap(...);
> munmap (data2,...);
> mmap (data2, ... MAP_FIXED)
>
> and get success on cygwin.

Yes, but basically only if you unmap the entire mmaped region.  See
below.

>   So I will be updating autoconf accordingly,
> based on the STD below.  Unfortunately, it looks like I also found a hole
> in cygwin.  Consider this (borrowing heavily from the autoconf test that I
> am fixing):
> [...]
> This test behaves differently on Linux than on cygwin; on Linux, both
> './foo' and './foo 1' give status 0, but on cygwin, './foo' gives status
> 6, and only './foo 1' succeeds.  In other words, the second mmap fails if
> there is no intermediate munmap.
>
> POSIX apparently allows cygwin's behavior:
>
> "If MAP_FIXED is set, mmap() may return MAP_FAILED and set errno to
> [EINVAL]. If a MAP_FIXED request is successful, the mapping established by
> mmap() replaces any previous mappings for the pages in the range
> [pa,pa+len) of the process."
>
> However, since we already have to maintain a list of mappings in order to
> implement fork(), it seems like it would be easy to fix cygwin to
> implicitly munmap anything that would otherwise be in the way of a
> subsequent MAP_FIXED request, rather than blindly calling
> NtMapViewOfSection and failing because of the overlap, so that we could be
> even more like Linux behavior.

That's tricky and bound to fail.  The problem is that, in Windows,
you can't munmap mmap'ed regions only partially.  NtUnmapViewOfSection
only allows to unmap an entire section.  So, with the bookkeeping in
Cygwin you can re-use a partially unmapped region of anonymous
memory to map new anonymous memory, but you can't reuse a partially
unmapped region to mmap another file at this point in memory, nor
even the same file with just another offset.

The only way around this problem would be to map files and anonymous
memory always in single 64K chunks, so that every page of a map can be
actually unmapped on OS level.  But in that case the process of allocating
memory is not atomic anymore, so we get the other potential problem of
not being able to fulfill a request because another thread has called
VirtualAlloc one way or the other.

> > That's why I think we need at least two tests in autoconf, a generic
> > mmap test and a mmap test for the "mmap private/shared fixed at
> > somewhere already mapped" case, if an application actually insists on
> > using that.
>
> In the case of the autoconf test, I think a single test is still
> sufficient, once it is fixed to be portable to what POSIX requires.

One problem is actually grep, which started the entire discussion.  It
really uses malloc/mmap(MAP_FIXED), along the lines of what the HAVE_MMAP
test tests.  Fortunately, grep doesn't fail if mmap returns an error, so
it doesn't hurt.  Of course it would be nice if grep would use mmap in
a more portable way.


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple