Ramsay Jones <ram...@ramsay1.demon.co.uk> writes:
> Thomas Gummerer wrote:
>> Hi,
>>
>> previous rounds (without api) are at $gmane/202752, $gmane/202923,
>> $gmane/203088 and $gmane/203517, the previous round with api was at
>> $gmane/229732. Thanks to Junio, Duy and Eric for their comments on
>> the previous round.
>
> If I remember correctly, the original version of this series had the
> same problem as Michael's "Fix some reference-related races" series
> (now in master). In particular, you had introduced an 'index_changed()'
> function which does essentially the same job as 'stat_validity_check()'
> in the new reference handling API. I seem to remember advising you
> not to compare st_uid, st_gid and st_ino on __CYGWIN__.
Yes, you provided a patch that simply wrapped those checks in a #if
!defined (__CYGWIN__), which is included in the new series too.
> I haven't had time to look at this version of your series yet, but it
> may be worth taking a look at stat_validity_check(). (although that is
> causing failures on cygwin at the moment! ;-)
I took a quick look, that function makes sense I think. I'll use it in
the re-roll. It makes probably sense to wrap the uid, gid and ino
fields as in the index_changed function.
> Also, I can't recall if I mentioned it to you at the time, but your
> index reading code was (unnecessarily) calling munmap() twice on the
> same buffer (without an intervening mmap()). This causes problems for
> systems that have the NO_MMAP build variable set. In particular, the
> compat/mmap.c code will attempt to free() the allocated memory block
> twice, with unpredictable results.
>
> I wrote a patch to address this at the time (Hmm, seems to be built
> on v1.8.1), but didn't submit it since your patch didn't progress. :-D
> I have included the patch below.
I can't recall this either. From a quick check I don't call munmap() on
a already unmapped mmap, so I think this is fine as it is and your patch
is independent from it. Not sure if it makes sense as safeguard for
future changes.
> -- >8 --
> From: Ramsay Jones <ram...@ramsay1.demon.co.uk>
> Date: Sun, 9 Sep 2012 20:50:32 +0100
> Subject: [PATCH] mmap.c: Keep log of mmap() blocks to avoid double-delete bug
>
> When compiling with the NO_MMAP build variable set, the built-in
> 'git_mmap()' and 'git_munmap()' compatibility routines use simple
> memory allocation and file I/O to emulate the required behaviour.
> The current implementation is vulnerable to the "double-delete" bug
> (where the pointer returned by malloc() is passed to free() two or
> more times), should the mapped memory block address be passed to
> munmap() multiple times.
>
> In order to guard the implementation from such a calling sequence,
> we keep a list of mmap-block descriptors, which we then consult to
> determine the validity of the input pointer to munmap(). This then
> allows 'git_munmap()' to return -1 on error, as required, with
> errno set to EINVAL.
>
> Using a list in the log of mmap-ed blocks, along with the resulting
> linear search, means that the performance of the code is directly
> proportional to the number of concurrently active memory mapped
> file regions. The number of such regions is not expected to be
> excessive.
>
> Signed-off-by: Ramsay Jones <ram...@ramsay1.demon.co.uk>
> ---
> compat/mmap.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/compat/mmap.c b/compat/mmap.c
> index c9d46d1..400e034 100644
> --- a/compat/mmap.c
> +++ b/compat/mmap.c
> @@ -1,14 +1,61 @@
> #include "../git-compat-util.h"
>
> +struct mmbd { /* memory mapped block descriptor */
> + struct mmbd *next; /* next in list */
> + void *start; /* pointer to memory mapped block */
> + size_t length; /* length of memory mapped block */
> +};
> +
> +static struct mmbd *head; /* head of mmb descriptor list */
> +
> +
> +static void add_desc(struct mmbd *desc, void *start, size_t length)
> +{
> + desc->start = start;
> + desc->length = length;
> + desc->next = head;
> + head = desc;
> +}
> +
> +static void free_desc(struct mmbd *desc)
> +{
> + if (head == desc)
> + head = head->next;
> + else {
> + struct mmbd *d = head;
> + for (; d; d = d->next) {
> + if (d->next == desc) {
> + d->next = desc->next;
> + break;
> + }
> + }
> + }
> + free(desc);
> +}
> +
> +static struct mmbd *find_desc(void *start)
> +{
> + struct mmbd *d = head;
> + for (; d; d = d->next) {
> + if (d->start == start)
> + return d;
> + }
> + return NULL;
> +}
> +
> void *git_mmap(void *start, size_t length, int prot, int flags, int fd,
> off_t offset)
> {
> size_t n = 0;
> + struct mmbd *desc = NULL;
>
> if (start != NULL || !(flags & MAP_PRIVATE))
> die("Invalid usage of mmap when built with NO_MMAP");
>
> start = xmalloc(length);
> - if (start == NULL) {
> + desc = xmalloc(sizeof(*desc));
> + if (!start || !desc) {
> + free(start);
> + free(desc);
> errno = ENOMEM;
> return MAP_FAILED;
> }
> @@ -25,18 +72,26 @@ void *git_mmap(void *start, size_t length, int prot, int
> flags, int fd, off_t of
> if (errno == EAGAIN || errno == EINTR)
> continue;
> free(start);
> + free(desc);
> errno = EACCES;
> return MAP_FAILED;
> }
>
> n += count;
> }
> + add_desc(desc, start, length);
>
> return start;
> }
>
> int git_munmap(void *start, size_t length)
> {
> + struct mmbd *d = find_desc(start);
> + if (!d) {
> + errno = EINVAL;
> + return -1;
> + }
> + free_desc(d);
> free(start);
> return 0;
> }
> --
> 1.8.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
