From: "Ondřej Bílka" <neleai@seznam.cz>
To: "Torsten Bögershausen" <tboegi@web.de>
Cc: "Antoine Pelisse" <apelisse@gmail.com>,
git@vger.kernel.org, "Wataru Noguchi" <wnoguchi.0727@gmail.com>,
"Erik Faye-Lund" <kusmabite@gmail.com>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"René Scharfe" <l.s.r@web.de>, msysGit <msysgit@googlegroups.com>
Subject: Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
Date: Sun, 20 Oct 2013 09:39:05 +0200 [thread overview]
Message-ID: <20131020073905.GA10718@domone.podge> (raw)
In-Reply-To: <526377C1.8020907@web.de>
On Sun, Oct 20, 2013 at 08:27:13AM +0200, Torsten Bögershausen wrote:
> Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on
> popelka.ms.mff.cuni.cz
> Status: O
> Content-Length: 2690
> Lines: 89
>
> On 20.10.13 08:05, Ondřej Bílka wrote:
> > On Sun, Oct 20, 2013 at 07:47:06AM +0200, Torsten Bögershausen wrote:
> >> (may be s/path is too big/path is too long/ ?)
> >>
> >> On 19.10.13 12:52, Antoine Pelisse wrote:
> >>> Currently, most buffers created with PATH_MAX length, are not checked
> >>> when being written, and can overflow if PATH_MAX is not big enough to
> >>> hold the path.
> >>>
> >>> Fix that by using strlcpy() where strcpy() was used, and also run some
> >>> extra checks when copy is done with memcpy().
> >>>
> >>> Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
> >>> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> >>> ---
> >>> diff --git a/abspath.c b/abspath.c
> >>> index 64adbe2..0e60ba4 100644
> >>> --- a/abspath.c
> >>> +++ b/abspath.c
> >>> @@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
> >>> const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> >>> {
> >>> static char path[PATH_MAX];
> >
> > Why do you need static there?
> Good point.
> get_pathname() from path.c may be better.
>
> >>> +
> >>> + if (pfx_len > PATH_MAX)
> >> I think this should be
> >> if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0'
> >>> + die("Too long prefix path: %s", pfx);
> >>> +
> >>> #ifndef GIT_WINDOWS_NATIVE
> >>> if (!pfx_len || is_absolute_path(arg))
> >>> return arg;
> >>> memcpy(path, pfx, pfx_len);
> >>> - strcpy(path + pfx_len, arg);
> >>> + strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
> >>
> >> I'm not sure how to handle overlong path in general, there are several ways:
> >> a) Silently overwrite memory (with help of memcpy() and/or strcpy()
> >> b) Silently shorten the path using strlcpy() instead of strcpy()
> >> c) Avoid the overwriting and call die().
> >> d) Prepare a longer buffer using xmalloc()
> >>
> > There is also
> > e) modify allocation to place write protected page after buffer end.
>
> Yes, I think this is what electric fence, DUMA or valgrind do:
>
You need to be selective which buffers are important.
> http://sourceforge.jp/projects/freshmeat_efence/
> http://duma.sourceforge.net/
These are toys, this comes with fact that they need a 8kb of space for
each 8byte malloc. Just run a git diff and differences are huge.
$ /usr/bin/time git diff HEAD^ > x
0.06user 0.01system 0:00.07elapsed 97%CPU (0avgtext+0avgdata
19172maxresident)k
0inputs+8outputs (0major+5591minor)pagefaults 0swaps
$ LD_PRELOAD=libefence.so /usr/bin/time git diff HEAD^ > x
Electric Fence 2.2 Copyright (C) 1987-1999 Bruce Perens
<bruce@perens.com>
3.49user 0.94system 0:04.45elapsed 99%CPU (0avgtext+0avgdata
91920maxresident)k
0inputs+8outputs (0major+118069minor)pagefaults 0swaps
> http://valgrind.sourceforge.net/
>
> Theses are very good tools for developers, finding memory corruption
> (or other bugs like using uninitialized memory).
>
> One of the motivation I asked for test cases is that a git developer can
> run these test cases under valgrind and can verify that we are never out of range.
>
> For an end user a git "crash" caused by trying to write to a write protected page
> is better than silently corrupting memory.
>
> And a range check, followed by die(), is even easier to debug.
> For an end user.
> /Torsten
>
next prev parent reply other threads:[~2013-10-20 7:39 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-28 21:17 [PATCH] mingw-multibyte: fix memory acces violation and path length limits Wataru Noguchi
2013-09-28 23:18 ` Johannes Schindelin
2013-09-29 2:56 ` Wataru Noguchi
2013-09-29 11:01 ` [msysGit] " Stefan Beller
2013-10-01 13:37 ` Wataru Noguchi
2013-09-30 17:00 ` René Scharfe
2013-09-30 21:02 ` Erik Faye-Lund
2013-10-01 13:35 ` Wataru Noguchi
2013-10-02 22:26 ` Wataru Noguchi
2013-10-03 17:25 ` Antoine Pelisse
2013-10-03 17:36 ` Erik Faye-Lund
2013-10-05 11:39 ` Wataru Noguchi
2013-10-19 10:52 ` [PATCH] Prevent buffer overflows when path is too big Antoine Pelisse
2013-10-20 5:47 ` Torsten Bögershausen
2013-10-20 6:05 ` [msysGit] " Ondřej Bílka
2013-10-20 6:27 ` Torsten Bögershausen
2013-10-20 7:39 ` Ondřej Bílka [this message]
2013-10-20 10:33 ` Duy Nguyen
2013-10-20 17:57 ` Antoine Pelisse
2013-10-21 1:31 ` Duy Nguyen
2013-10-21 19:02 ` Johannes Sixt
2013-10-21 19:07 ` Erik Faye-Lund
2013-10-21 19:14 ` Jeff King
2013-10-21 19:32 ` Jeff King
2013-10-23 12:55 ` [PATCH 1/2] entry.c: convert checkout_entry to use strbuf Nguyễn Thái Ngọc Duy
2013-10-23 12:55 ` [PATCH 2/2] entry.c: convert write_entry " Nguyễn Thái Ngọc Duy
2013-10-23 17:52 ` Junio C Hamano
2013-10-24 1:23 ` Duy Nguyen
2013-10-24 19:49 ` Junio C Hamano
2013-10-24 23:47 ` Duy Nguyen
2013-10-23 12:58 ` [PATCH 1/2] entry.c: convert checkout_entry " Antoine Pelisse
2013-10-23 13:04 ` Duy Nguyen
2013-10-23 13:06 ` Antoine Pelisse
2013-10-23 17:29 ` Jeff King
2013-10-23 17:34 ` Erik Faye-Lund
2013-10-23 17:52 ` Jeff King
2013-10-23 18:09 ` Junio C Hamano
2013-10-23 18:10 ` Jeff King
2013-10-24 1:55 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2013-10-23 12:55 ` [PATCH] Prevent buffer overflows when path is too big Duy Nguyen
2013-11-26 18:39 ` [PATCH] Prevent buffer overflows when path is too long Antoine Pelisse
2013-11-26 19:50 ` Junio C Hamano
2013-11-29 12:12 ` Antoine Pelisse
2013-12-14 11:31 ` Antoine Pelisse
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=20131020073905.GA10718@domone.podge \
--to=neleai@seznam.cz \
--cc=Johannes.Schindelin@gmx.de \
--cc=apelisse@gmail.com \
--cc=git@vger.kernel.org \
--cc=kusmabite@gmail.com \
--cc=l.s.r@web.de \
--cc=msysgit@googlegroups.com \
--cc=tboegi@web.de \
--cc=wnoguchi.0727@gmail.com \
/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).