From: Thomas Gummerer <t.gummerer@gmail.com>
To: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Cc: git@vger.kernel.org, trast@inf.ethz.ch, mhagger@alum.mit.edu,
gitster@pobox.com, pclouds@gmail.com, robin.rosenberg@dewire.com,
sunshine@sunshineco.com
Subject: Re: [PATCH v2 00/19] Index-v5
Date: Wed, 17 Jul 2013 10:04:15 +0200 [thread overview]
Message-ID: <87zjtlsjcw.fsf@gmail.com> (raw)
In-Reply-To: <51E5B52C.7050603@ramsay1.demon.co.uk>
Ramsay Jones <ramsay@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 <ramsay@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 <ramsay@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
prev parent reply other threads:[~2013-07-17 8:04 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-12 17:26 [PATCH v2 00/19] Index-v5 Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 01/19] t2104: Don't fail for index versions other than [23] Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 02/19] read-cache: split index file version specific functionality Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 03/19] read-cache: move index v2 specific functions to their own file Thomas Gummerer
2013-07-14 3:10 ` Duy Nguyen
2013-07-19 14:53 ` Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 04/19] read-cache: Re-read index if index file changed Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 05/19] Add documentation for the index api Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 06/19] read-cache: add index reading api Thomas Gummerer
2013-07-14 3:21 ` Duy Nguyen
2013-07-12 17:26 ` [PATCH v2 07/19] make sure partially read index is not changed Thomas Gummerer
2013-07-14 3:29 ` Duy Nguyen
2013-07-17 12:56 ` Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 08/19] grep.c: Use index api Thomas Gummerer
2013-07-14 3:32 ` Duy Nguyen
2013-07-15 9:51 ` Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 09/19] ls-files.c: use " Thomas Gummerer
2013-07-14 3:39 ` Duy Nguyen
2013-07-17 8:07 ` Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 10/19] documentation: add documentation of the index-v5 file format Thomas Gummerer
2013-07-14 3:59 ` Duy Nguyen
2013-07-17 8:09 ` Thomas Gummerer
2013-08-04 11:26 ` Duy Nguyen
2013-08-04 17:58 ` Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 11/19] read-cache: make in-memory format aware of stat_crc Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 12/19] read-cache: read index-v5 Thomas Gummerer
2013-07-14 4:42 ` Duy Nguyen
2013-08-07 8:13 ` Thomas Gummerer
2013-07-15 10:12 ` Duy Nguyen
2013-07-17 8:11 ` Thomas Gummerer
2013-08-08 2:00 ` Duy Nguyen
2013-08-08 13:28 ` Thomas Gummerer
2013-08-09 13:10 ` Thomas Gummerer
2013-08-07 8:23 ` Thomas Gummerer
2013-08-08 2:09 ` Duy Nguyen
2013-07-12 17:26 ` [PATCH v2 13/19] read-cache: read resolve-undo data Thomas Gummerer
2013-07-12 17:26 ` [PATCH v2 14/19] read-cache: read cache-tree in index-v5 Thomas Gummerer
2013-07-12 17:27 ` [PATCH v2 15/19] read-cache: write index-v5 Thomas Gummerer
2013-07-12 17:27 ` [PATCH v2 16/19] read-cache: write index-v5 cache-tree data Thomas Gummerer
2013-07-12 17:27 ` [PATCH v2 17/19] read-cache: write resolve-undo data for index-v5 Thomas Gummerer
2013-07-12 17:27 ` [PATCH v2 18/19] update-index.c: rewrite index when index-version is given Thomas Gummerer
2013-07-12 17:27 ` [PATCH v2 19/19] p0003-index.sh: add perf test for the index formats Thomas Gummerer
2013-07-14 2:59 ` [PATCH v2 00/19] Index-v5 Duy Nguyen
2013-07-15 9:30 ` Thomas Gummerer
2013-07-15 9:38 ` Duy Nguyen
2013-07-17 8:12 ` Thomas Gummerer
2013-07-17 23:58 ` Junio C Hamano
2013-07-19 17:37 ` Thomas Gummerer
2013-07-19 18:25 ` Junio C Hamano
2013-07-16 21:03 ` Ramsay Jones
2013-07-17 8:04 ` Thomas Gummerer [this message]
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zjtlsjcw.fsf@gmail.com \
--to=t.gummerer@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
--cc=pclouds@gmail.com \
--cc=ramsay@ramsay1.demon.co.uk \
--cc=robin.rosenberg@dewire.com \
--cc=sunshine@sunshineco.com \
--cc=trast@inf.ethz.ch \
/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 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).