[PATCH] Fix for issue 365: "Out of memory? mmap failed"

[Erik Faye-Lund]

From: Ian McLean <ian.m...@gmail.com>

The git_mmap implementation was broken for file sizes that
wouldn't fit into a size_t (32 bits). Changed 'len' to be stored
in an off_t (64 bit) variable, and moved the xsize_t cast to only
occur when an attempt is made to map past the end-of-file.
---

Originally from, the issue tracker, issue 365:
http://code.google.com/p/msysgit/issues/detail?id=365

It's about time to get this patch discussed properly, and the
involved parties doesn't seem to be interested enough to do
anything about it.

I didn't modify it from what Ian originally attached to the issue
tracker, mostly because I didn't understand the issue Dscho
mentioned; I believe mmap's length-parameter should be size_t,
because it specifies the amout of memory to map into our process.
We can't map more memory into our process than the size of the
address-space, no?

OpenGroup seems to agree with me:
http://www.opengroup.org/onlinepubs/007908775/xsh/mmap.html

In other words: I probably misunderstood Dscho's feed-back, but
I can't figure out what he meant in that case :P

compat/win32mmap.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/compat/win32mmap.c b/compat/win32mmap.c
index 1c5a149..b58aa69 100644
--- a/compat/win32mmap.c
+++ b/compat/win32mmap.c
@@ -4,19 +4,19 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
{
HANDLE hmap;
void *temp;
- size_t len;
+ off_t len;
struct stat st;
uint64_t o = offset;
uint32_t l = o & 0xFFFFFFFF;
uint32_t h = (o >> 32) & 0xFFFFFFFF;

if (!fstat(fd, &st))
- len = xsize_t(st.st_size);
+ len = st.st_size;
else
die("mmap: could not determine filesize");

if ((length + offset) > len)
- length = len - offset;
+ length = xsize_t(len - offset);

if (!(flags & MAP_PRIVATE))
die("Invalid usage of mmap when built with USE_WIN32_MMAP");
--
1.7.0.3.2243.g153c6

[Ian McLean]

Thanks for greasing the wheels on this issue, Erik. Sorry I let the
patch languish on the tracker -- busy time. I've been running our
team with the aforementioned patch for the last few months (our repo
pack size is currently at 5.5GB) and it's been working for us.

To muddy the issue a bit, when the repo pack size gets over 4GB, and
the mmap problem occurs, there's likely to also be other problems
(bugs in mainline git) that show up. Mostly due to large (200MB+)
commits and address space fragmentation during windowed compression.
These cause out-of-memory conditions that happen in mainline linux
builds, but are somewhat more prevalent in msysgit due to smaller
available process address space (2GB vs 3GB in linux). The result is
the same as the mmap 4GB implementation error: out of memory, mmap
failed.

The xsize_t cast function is quite scary -- have a grep of the
mainline git source; there's several occurrences of it there that
raise a questioning eyebrow. But those are upstream issues... this
patch addresses the only xsize_t cast in msysgit-specific code, so I
think it's at least a step in the right direction.

-Ian

[Johannes Schindelin]

Hi,

On Mon, 12 Apr 2010, Erik Faye-Lund wrote:

> From: Ian McLean <ian.m...@gmail.com>
>
> The git_mmap implementation was broken for file sizes that
> wouldn't fit into a size_t (32 bits). Changed 'len' to be stored
> in an off_t (64 bit) variable, and moved the xsize_t cast to only
> occur when an attempt is made to map past the end-of-file.
> ---

I did not really wrap my head around all the details, but I cherry-picked
it to devel. Might just as well cook it there.

Thanks,
Dscho


--
Subscription settings: http://groups.google.com/group/msysgit/subscribe?hl=en

[Johannes Sixt]

On Montag, 12. April 2010, Erik Faye-Lund wrote:
> From: Ian McLean <ian.m...@gmail.com>
>
> The git_mmap implementation was broken for file sizes that
> wouldn't fit into a size_t (32 bits). Changed 'len' to be stored
> in an off_t (64 bit) variable, and moved the xsize_t cast to only
> occur when an attempt is made to map past the end-of-file.
> ---
>
> Originally from, the issue tracker, issue 365:
> http://code.google.com/p/msysgit/issues/detail?id=365
>
> It's about time to get this patch discussed properly, and the
> involved parties doesn't seem to be interested enough to do
> anything about it.
>
> I didn't modify it from what Ian originally attached to the issue
> tracker, mostly because I didn't understand the issue Dscho
> mentioned; I believe mmap's length-parameter should be size_t,
> because it specifies the amout of memory to map into our process.
> We can't map more memory into our process than the size of the
> address-space, no?
>
> OpenGroup seems to agree with me:
> http://www.opengroup.org/onlinepubs/007908775/xsh/mmap.html
>
> In other words: I probably misunderstood Dscho's feed-back, but
> I can't figure out what he meant in that case :P

Dscho said:
> Unfortunately, this patch is not uncontroversial. [...] The "length"
> variable is still a size_t, and if that is different than off_t, the
> parameter of mmap() cannot be a size_t.

IMO, the parameters of mmap are fine: offset is 64bit off_t, so it can pointer
anywhere in files larger than 4GB. It is OK that the length parameter is only
32bit size_t because it specifies the amount of data to map (not the size of
the file), and we cannot have more than 2^32 anyway in a 32bit program.

For me, the patch looks right. It's been in devel of 4msysgit for a while.
Does this mean that people have used it in production? I forgot to carry it
in my tree, so I can't vouch for anything, but would like to submit it
together with the patches I mentioned yesterday.

-- Hannes