git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* Regression: git no longer works with musl libc's regex impl
@ 2016-10-04 15:08 Rich Felker
  2016-10-04 15:27 ` Jeff King
  0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2016-10-04 15:08 UTC (permalink / raw)
  To: git; +Cc: musl

This commit broke support for using git with musl libc:

https://github.com/git/git/commit/2f8952250a84313b74f96abb7b035874854cf202

Rather than depending on non-portable GNU regex extensions, there is a
simple portable fix for the issue this code was added to work around:
When a text file is being mmapped for use with string functions which
depend on null termination, if the file size:

1. is nonzero mod page size, it just works; the remainder of the last
   page reads as zero bytes when mmapped.

2. if an exact multiple of the page size, then instead of directly
   mmapping the file, first mmap a mapping 1 byte (thus 1 page) larger
   with MAP_ANON, then use MAP_FIXED to map the file over top of all
   but the last page. Now the mmapped buffer can safely be used as a C
   string.

If such a solution is acceptable I can try to prepare a patch.

Rich

^ permalink raw reply	[flat|nested] 31+ messages in thread
* RE: [musl] Re: Regression: git no longer works with musl libc's regex impl
@ 2016-10-05  3:00 writeonce
  2016-10-05 10:49 ` Johannes Schindelin
  0 siblings, 1 reply; 31+ messages in thread
From: writeonce @ 2016-10-05  3:00 UTC (permalink / raw)
  To: musl; +Cc: Johannes.Schindelin, git, Jeff King

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


< 
< 
< -------- Original Message --------
< Subject: [musl] Re: Regression: git no longer works with musl libc's
< regex impl
< From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
< Date: Tue, October 04, 2016 9:08 am
< To: Rich Felker <dalias@libc.org>
< Cc: Jeff King <peff@peff.net>, git@vger.kernel.org,
< musl@lists.openwall.com
< 
< Hi Rich,
< 
< On Tue, 4 Oct 2016, Rich Felker wrote:
< 
< > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
< > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
< > > 
< > > > 1. is nonzero mod page size, it just works; the remainder of the
last
< > > > page reads as zero bytes when mmapped.
< > > 
< > > Is that a portable assumption?
< > 
< > Yes.
< 
< No, it is not. You quote POSIX, but the matter of the fact is that we
use
< a subset of POSIX in order to be able to keep things running on
Windows.
< 

As far as I can tell (and as the attached program may help demonstrate),
the above assumption has been valid on all versions of Windows since at
least Windows 2000. In this context, one thing to remember is that the
page-size for the mod operation is 4096, whereas the POSIX page-size
(for the purpose of mmap and mremap) is 65536. Note also that in the
case of file-backed mapped sections, using kernel32.dll or msvcrt.dll or
cygwin/newlib or midipix/musl is of little significance, specifically
since all invoke ZwCreateSection and ZwMapViewOfSection under the hood.

HTH,
midipix

< And quite honestly, there are lots of reasons to keep things running
on
< Windows, and even to favor Windows support over musl support. Over
four
< million reasons: the Git for Windows users.
< 
< So rather than getting into an ideological discussion about "broken"
< systems, it would be good to keep things practical, realizing that
those
< users make up a very real chunk of all of Git's users.
< 
< As to making NO_REGEX conditional on REG_STARTEND: you are talking
about
< apples and oranges here. NO_REGEX is a Makefile flag, while
REG_STARTEND
< is a C preprocessor macro.
< 
< Unless you can convince the rest of the Git developers (you would not
< convince me) to simulate autoconf by compiling an executable every
time
< `make` is run, to determine whether REG_STARTEND is defined, this is a
< no-go.
< 
< However, you *can* use autoconf directly, and come up with a patch to
our
< configure.ac that detects the absence of REG_STARTEND and sets
NO_REGEX=1.
< 
< Alternatively, you can set NO_REGEX=1 in your config.mak.
< 
< Or, if you use one of the auto-detected cases in config.mak.uname, you
< could patch it to set NO_REGEX=1.
< 
< And lastly, the best alternative would be to teach musl about
< REG_STARTEND, as it is rather useful a feature.
< 
< Ciao,
< Johannes
< 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mmap_file.c --]
[-- Type: text/x-c; name="mmap_file.c";, Size: 1487 bytes --]

#define _XOPEN_SOURCE            500
#define NT_FILE_BACKED_PAGE_SIZE 4096

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <errno.h>
#include <sys/mman.h>

static int test_file_backed_mmap(int size)
{
	int     fd;
	int     exsize;
	void *  addr;
	char *  test;
	char *  cap;
	char    name[12];

	/* create test file of desired size */
	sprintf(name,"%d.tmp",size);

	if ((fd = creat(name,0755)) < 0)
		return -1;

	if (ftruncate(fd,size))
		return -1;

	close(fd);

	/* write 'W' to all bytes of test file */
	if ((fd = open(name,O_RDWR,0)) < 0)
		return -1;

	if ((addr = mmap(0,size,PROT_WRITE,MAP_SHARED,fd,0)) == MAP_FAILED)
		return -1;

	close(fd);

	test = (char *)addr;
	cap  = (char *)addr + size;

	for (; test<cap; test++)
		*test = 'W';

	munmap(addr,size);

	/* map file for read access */
	if ((fd = open(name,O_RDONLY,0)) < 0)
		return -1;

	if ((addr = mmap(0,size,PROT_READ,MAP_PRIVATE,fd,0)) == MAP_FAILED)
		return -1;

	close(fd);

	/* test */
	exsize = size + NT_FILE_BACKED_PAGE_SIZE;
	exsize &= ~(NT_FILE_BACKED_PAGE_SIZE - 1);

	test = (char *)addr + size;
	cap  = (char *)addr + exsize;

	for (; test<cap; test++)
		if (*test)
			return -1;

	/* clean-up */
	munmap(addr,size);
	unlink(name);

	return 0;

}

int main(void)
{
	int i;

	for (i=1; i<65536; i++)
		if (i % NT_FILE_BACKED_PAGE_SIZE)
			if (test_file_backed_mmap(i))
				return 2;

	return 0;
}

^ permalink raw reply	[flat|nested] 31+ messages in thread
* RE: [musl] Re: Regression: git no longer works with musl libc's regex impl
@ 2016-10-05 16:37 writeonce
  0 siblings, 0 replies; 31+ messages in thread
From: writeonce @ 2016-10-05 16:37 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: musl, git, Jeff King

Hi Johannes,

> 
> 
> -------- Original Message --------
> Subject: RE: [musl] Re: Regression: git no longer works with musl libc's
> regex impl
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Date: Wed, October 05, 2016 3:49 am
> To: writeonce@midipix.org
> Cc: musl@lists.openwall.com, git@vger.kernel.org, Jeff King
> <peff@peff.net>
> 
> Hi writeonce,
> 
> On Tue, 4 Oct 2016, writeonce@midipix.org wrote:
> 
> > < On Tue, 4 Oct 2016, Rich Felker wrote:
> > < 
> > < > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
> > < > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
> > < > > 
> > < > > > 1. is nonzero mod page size, it just works; the remainder of the
> > < > > > last page reads as zero bytes when mmapped.
> > < > > 
> > < > > Is that a portable assumption?
> > < > 
> > < > Yes.
> > < 
> > < No, it is not. You quote POSIX, but the matter of the fact is that we
> > < use a subset of POSIX in order to be able to keep things running on
> > < Windows.
> > 
> > As far as I can tell (and as the attached program may help demonstrate),
> > the above assumption has been valid on all versions of Windows since at
> > least Windows 2000.
> 
> And since W2K is already past its end of life, it would be safe for
> practical considerations.
> 
> However, I have to add two comments to that:
> 
> - it is *not* guaranteed. The behavior is undefined, even if you see
>  consistent behavior so far. Future Windows versions might break that
>  assumption freely, though.
> 

That is of course the official language, and generally speaking a good
rule of thumb. However... there is enough information to suggest that
when it comes to mapping of file-backed sections, the NT kernel
developers will choose to keep things the way they are. In brief, here's
why:

Given a "gray zone" that spans from EOF to end-of-page, there are in
essence three possible behaviors:

[1] bytes in the gray zone are accessible and are all zero's. This is
the current behavior.
[2] bytes in the gray zone are not accessible; trying to read past EOF
would result in a segfault.
[3] bytes in the gray zone are accessible but might contain random data
or junk.

Assessment:

[1] backward-compatible, POSIX-compliant, single code path for both
WIN32 and LXW.
[2] requires changing memory access granularity from 4096 bytes to a
single byte, and is therefore extremely costly.
[3] introduces a whole new class of security vulnerabilities, and will
thus be a lot of fun to watch:-)

All in all taken, then, I'd argue that relying on the current behavior
is very reasonable. If you, too, find the above assessment valid, and
since you mentioned that you were a Microsoft employee, it would be
great if you could make a good-faith effort to have the current behavior
added to the Driver Documentation and thus guaranteed.

PS. this isn't to say that the regex extension should or should not be
used, only that a decision on the matter should not be based on the
undocumentedness of current behavior.

midipix

> - some implementations of the REG_STARTEND feature have the nice property
>  that they can read past NUL characters. Granted, not all of them do
>  (AFAIU one example is FreeBSD itself, the first platform to sport
>  REG_STARTEND), but we at least reap the benefit whenever using a regex
>  that *can* read past NUL characters.
> 
> > In this context, one thing to remember is that the page-size for the mod
> > operation is 4096, whereas the POSIX page-size (for the purpose of mmap
> > and mremap) is 65536.
> 
> Indeed. A colleague of mine spotted the segfault when diffing a file that
> was *exactly* 4,096 bytes.
> 
> > Note also that in the case of file-backed mapped sections, using
> > kernel32.dll or msvcrt.dll or cygwin/newlib or midipix/musl is of little
> > significance, specifically since all invoke ZwCreateSection and
> > ZwMapViewOfSection under the hood.
> 
> Right. It's all backed by the very same kernel functions.
> 
> Ciao,
> Johannes


^ permalink raw reply	[flat|nested] 31+ messages in thread
end of thread, other threads:[~2016-10-07 11:31 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-04 15:08 Regression: git no longer works with musl libc's regex impl Rich Felker
2016-10-04 15:27 ` Jeff King
2016-10-04 15:40   ` Rich Felker
2016-10-04 16:08     ` Johannes Schindelin
2016-10-04 16:11       ` Rich Felker
2016-10-04 17:16         ` Johannes Schindelin
2016-10-04 18:00           ` Ray Donnelly
2016-10-04 17:39       ` [musl] " Rich Felker
2016-10-05 11:17         ` Johannes Schindelin
2016-10-05 13:01           ` Szabolcs Nagy
2016-10-05 13:15           ` Rich Felker
2016-10-04 22:06       ` James B
2016-10-04 22:33         ` Rich Felker
2016-10-04 22:48           ` Junio C Hamano
2016-10-05 13:11           ` Jakub Narębski
2016-10-05 16:15             ` [musl] " Rich Felker
2016-10-05 10:41         ` Johannes Schindelin
2016-10-05 11:59           ` James B
2016-10-05 16:11             ` Jeff King
2016-10-05 16:27               ` Rich Felker
2016-10-06 10:44             ` Johannes Schindelin
2016-10-06 19:18       ` Ævar Arnfjörð Bjarmason
2016-10-06 19:23         ` Jeff King
2016-10-06 19:25           ` Rich Felker
2016-10-06 19:28             ` Jeff King
2016-10-06 22:42         ` Ramsay Jones
2016-10-07 11:30           ` Jakub Narębski
2016-10-04 16:01   ` Johannes Schindelin
  -- strict thread matches above, loose matches on Subject: below --
2016-10-05  3:00 [musl] " writeonce
2016-10-05 10:49 ` Johannes Schindelin
2016-10-05 16:37 writeonce

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).