* Re: Wanted - a file browser interface to git
From: Linus Torvalds @ 2005-10-19 1:07 UTC (permalink / raw)
To: John Ellson; +Cc: git
In-Reply-To: <dj45np$e88$1@sea.gmane.org>
On Tue, 18 Oct 2005, John Ellson wrote:
>
> An example is: "I know that file xxx contained algorithm yyy at some point in
> the past and now I'd like to browse back through the history of xxx to find
> the exact details."
You are aware of "git whatchanged -p xxx", right?
Yeah, it's not graphical, and I agree that it might be very cool to have a
graphical version of it. But I thought I'd mention it even so. A
surprising number of people seem to have never realized, and at least for
me personally, it's one of the most common things I do.
Linus
^ permalink raw reply
* Re: gitweb.cgi
From: Linus Torvalds @ 2005-10-19 1:02 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Kay Sievers, Git Mailing List
In-Reply-To: <43559575.1060902@zytor.com>
On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> It turns out that the default CacheSize is only 256K. D'oh! Fixed.
>
> I also changed the CacheDefaultExpire to 600 seconds.
Ok, that sounds like it should improve things. My quick tests didn't seem
to show any difference, though. Do you need to re-load the apache module
or something?
> The only thing the front page really should need is to know when the last
> change to the tree was, which presumably means looking at each head of each
> tree and follow the chain until there is a datable object.
Yeah. I tried to follow gitweb.cgi, but I'm neither http- nor
perl-literate, so I'm not sure I caught everything.
But it does seem to basically end up doing a "git_read_commit()" for each
project, and that in turn was doing the "git-rev-list --max-count=1" thing
that I just sent out a suggested improvement for.
It effectively removes two or more copies of
stat64("/objects/xy/zzy", {...})
fd = open("objects/xy/zzy", O_RDONLY|O_NOATIME)
addr = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0)
close(fd)
munmap(addr, size)
which really should be very cheap operations, but hey, if the disk head is
somewhere else (and busy) and it's not cached, it can be quite expensive.
Especially since we don't end up usign the result.
I'm sure there's room for improvement inside gitweb itself too, but maybe
the git-rev-list optimization will help.
Linus
^ permalink raw reply
* Wanted - a file browser interface to git
From: John Ellson @ 2005-10-19 0:58 UTC (permalink / raw)
To: git
I know that Linus thinks that files are less important than commits, but we are
finding a real need to be able to browse though old versions of files and we
have not yet found an efficient way to do it with git.
An example is: "I know that file xxx contained algorithm yyy at some point in
the past and now I'd like to browse back through the history of xxx to find the
exact details."
I think what I'd like is a file browser on git that:
- can navigate the directory tree, starting by default with the HEAD
tree, but able to browse the state of the tree at any time in the history.
- can select any file from the tree, and then view the state of that file at any
time in its history by stepping forward or back through commits that have
affected that file.
- can view the difference between any pair of states of the file, with
annotations as to the source of the changes.
- can search for a string across the complete history of a file.
- can invoke the users choice of editor on the file.
Neither gitk nor qgit provide tree browsing, so it can be hard to get at a
specific file.
qgit has nice file browser that annotates all changes, but I think I'd prefer a
two panel diff view.
Neither qgit not gitk provide links to an editor so that a file can be worked on
once found.
Am I out in left field here, or does anyone else feel the need for something
like this?
John
^ permalink raw reply
* Optimize common case of git-rev-list (was Re: gitweb.cgi)
From: Linus Torvalds @ 2005-10-19 0:53 UTC (permalink / raw)
To: H. Peter Anvin, Junio C Hamano; +Cc: Kay Sievers, Git Mailing List
In-Reply-To: <43559399.2030903@zytor.com>
On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> > Considering that apparently the load is enough that it takes 45 seconds to
> > generate (scary in itself), is should clearly be cached for more than one
> > minute. More like ten minutes or half an hour, especially since mirroring
> > any content changes takes longer than that anyway.
>
> The latency for an I/O operation on the kernel.org servers is positively
> scary.
I took a look at webgit, and it looks like at least for the "projects"
page, the most common operation ends up being basically
git-rev-list --header --parents --max-count=1 HEAD
Now, the thing is, the way "git-rev-list" works, it always keeps on
popping the parents and parsing them in order to build the list of
parents, and it turns out that even though we just want a single commit,
git-rev-list will invariably look up _three_ generations of commits.
It will parse:
- the commit we want (it obviously needs this)
- it's parent(s) as part of the "pop_most_recent_commit()" logic
- it will then pop one of the parents before it notices that it doesn't
need any more
- and as part of popping the parent, it will parse the grandparent (again
due to "pop_most_recent_commit()".
Now, I've strace'd it, and it really is pretty efficient on the whole, but
if things aren't nicely cached, and with long-latency IO, doing those two
extra objects (at a minimum - if the parent is a merge it will be more) is
just wasted time, and potentially a lot of it.
So here's a quick special-case for the trivial case of "just one commit,
and no date-limits or other special rules".
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---
I've actually tried to test it (but hey, "exhaustive" is hard with all the
different options), and I tried to be very careful to only do the
special-case when really nothing can go wrong, and this all looks obvious.
But buyer beware.
Btw, doing an "strace" on git-rev-list shows that most of the system calls
by far are the dynamic loader. Now, those accesses _should_ all be cached,
so they should be fast and low-latency, but it's entirely possible that
for a server configuration you might want to actually link things
statically. Or not. Just a thought.
diff --git a/rev-list.c b/rev-list.c
index c60aa72..d4da1bd 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -624,6 +624,10 @@ int main(int argc, char **argv)
if (!merge_order) {
sort_by_date(&list);
+ if (list && !limited && max_count == 1) {
+ show_commit(list->item);
+ return 0;
+ }
if (limited)
list = limit_list(list);
if (topo_order)
^ permalink raw reply related
* Re: [PATCH] git-daemon extra paranoia
From: H. Peter Anvin @ 2005-10-19 0:43 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0510181728490.3369@g5.osdl.org>
Linus Torvalds wrote:
> And just appending ".git" is _not_ badly designed/specified. I did think
> about the boundary cases, and it's entirely safe:
>
> - it can't result in "surprises": if the original pathname doesn't exist,
> then even if there is a race and it got created in between the two
> chdir's as a directory and the name had a slash at the end, adding
> ".git" is actually safe even if it succeeds: it won't take us anywhere
> surprising. At worst it will take us to the ".git" directory of a newly
> added git archive, but that's what we wanted anyway, so..
>
> - you can't create ".." with it - even if the passed-in filename ended
> with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any
> safety checks that were done on the original pathname are still valid
> when appending ".git" to it.
>
> - and exactly because we don't append slashes or anything like that, the
> end result won't even have anything ambiguous like "//" in it.
>
> So it really doesn't have any downsides that I can see.
>
Consider the whitelist/blacklist scenario I described in the previous
email. You have:
whitelist: /pub/scm
blacklist: /pub/scm/foo/bar.git
If you can bypass the blacklist by using the pathname /pub/scm/foo/bar,
that's bad.
>
>>The DWIM aspect is fine, of course, but it has to be done up front: instead of
>>doing just chdir(), each path should be validated through path_ok() before
>>even being considered for chdir(). Perhaps the right thing to do is to
>>combine the two functions.
>
> Sure, you could do that, and just replace path_ok + chdir with a
> "safe_chdir()". I don't really see the point, unless you want to walk the
> path one component at a time, though (which is really quite expensive).
>
The only reason to do that is to make it less likely that a future
programmer would screw it up.
-hpa
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: Linus Torvalds @ 2005-10-19 0:41 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Git Mailing List
In-Reply-To: <435591A3.7030708@zytor.com>
On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> This is also exactly the kind of DWIM that tends to result in the kind of
> security holes I described earlier.
I don't agree.
DWIM isn't automatically a security hole. DWIM _can_ be a security hole,
but so can anything else that is badly designed or specified.
And just appending ".git" is _not_ badly designed/specified. I did think
about the boundary cases, and it's entirely safe:
- it can't result in "surprises": if the original pathname doesn't exist,
then even if there is a race and it got created in between the two
chdir's as a directory and the name had a slash at the end, adding
".git" is actually safe even if it succeeds: it won't take us anywhere
surprising. At worst it will take us to the ".git" directory of a newly
added git archive, but that's what we wanted anyway, so..
- you can't create ".." with it - even if the passed-in filename ended
with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any
safety checks that were done on the original pathname are still valid
when appending ".git" to it.
- and exactly because we don't append slashes or anything like that, the
end result won't even have anything ambiguous like "//" in it.
So it really doesn't have any downsides that I can see.
> The DWIM aspect is fine, of course, but it has to be done up front: instead of
> doing just chdir(), each path should be validated through path_ok() before
> even being considered for chdir(). Perhaps the right thing to do is to
> combine the two functions.
Sure, you could do that, and just replace path_ok + chdir with a
"safe_chdir()". I don't really see the point, unless you want to walk the
path one component at a time, though (which is really quite expensive).
If you want to verify that it's still on the same filesystem and didn't
traverse any dubious symlinks (the only reason to do the component walking
afaik), it's actually much cheaper to just do the chdir() and then do a
"getcwd()" to verify that the result matches. At least under Linux.
(That, btw, is likely the right way to do "valid directory checking"
anyway: if you have a white-list of acceptable directories, just do a
chdir() blindly without any checking, then do "getcwd()" and check the
result of that against the whitelist - then you can even allow ".." etc,
and never even care)
Linus
^ permalink raw reply
* Re: gitweb.cgi
From: H. Peter Anvin @ 2005-10-19 0:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Kay Sievers, Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0510181645200.3369@g5.osdl.org>
Linus Torvalds wrote:
>
> It really doesn't work very well with the "front page" though..
>
> Doing a "Save page as.." shows that it's not a huge page: it's roughly 700
> lines long and 57kB in size, but pressing the reload button (or just going
> somewhere else and coming back immediately) takes 45 seconds to reload for
> me.
>
> Trying again shows that it _is_ cached if you press the reload button
> immediately again, but I haven't quite figured out how long the cache
> timeout is. It seems to be around one minute (from some very preliminary
> tests it's more than 25 seconds, but less than a minute and a half).
>
It turns out that the default CacheSize is only 256K. D'oh! Fixed.
I also changed the CacheDefaultExpire to 600 seconds.
> That said, I tried to figure out how the front page is generated, but
> haven't quite. Can somebody (Kay?) please say what it does most, and I can
> try to make sure git does that efficiently..
The only thing the front page really should need is to know when the
last change to the tree was, which presumably means looking at each head
of each tree and follow the chain until there is a datable object.
-hpa
^ permalink raw reply
* Re: gitweb.cgi
From: H. Peter Anvin @ 2005-10-19 0:30 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Kay Sievers, Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0510181645200.3369@g5.osdl.org>
Linus Torvalds wrote:
>
> It really doesn't work very well with the "front page" though..
>
> Doing a "Save page as.." shows that it's not a huge page: it's roughly 700
> lines long and 57kB in size, but pressing the reload button (or just going
> somewhere else and coming back immediately) takes 45 seconds to reload for
> me.
>
> Trying again shows that it _is_ cached if you press the reload button
> immediately again, but I haven't quite figured out how long the cache
> timeout is. It seems to be around one minute (from some very preliminary
> tests it's more than 25 seconds, but less than a minute and a half).
>
The cache timeout is set to 300 seconds, however, that's per server, of
course.
> Considering that apparently the load is enough that it takes 45 seconds to
> generate (scary in itself), is should clearly be cached for more than one
> minute. More like ten minutes or half an hour, especially since mirroring
> any content changes takes longer than that anyway.
The latency for an I/O operation on the kernel.org servers is positively
scary.
-hpa
^ permalink raw reply
* Re: [CORRECTED PATCH] git-fetch-pack: avoid unnecessary zero packing
From: Junio C Hamano @ 2005-10-19 0:27 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0510181339220.3369@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> I see you already did. Looks fine. I'd suggest limiting the commits by
> number in mark_recent_commit_complete(), because
>
> (a) somebody might have their clock set wrong and you don't want to walk
> a huge tree just because of something like that.
> (b) you might just have imported a huge history (badly) from somewhere
> else
> (c) a _lot_ can happen in five days with automated things.
>
> but yes, the approach looks very sane otherwise.
When you have several dozen commits on top of a head you fetched
from the remote last time you polled them, and the remote has
not updated that head since then, it may be worthwhile to have
the client dig deeper to avoid asking the server.
What I am thinking is:
- For objects our refs directly refer to, mark them COMPLETE
as the patch I sent out.
- See if we have any objects the remote refs refer to
already; find the timestamp of the latest one if we have
commits among them, and use its time as the cutoff time.
It is likely that we have synched with them after that
timestamp (either upload or download). walk the commits
from our ref, and mark *everything* that are newer than
that timestamp. This can turn out to be a huge walking
but that happens on the client side.
This way I can get rid of the arbitrary 5-day window, and I do
not have to invent another arbitrary number to limit the commits
we walk.
In other words, let's put the burden on the client if its effort
possibly can help the server.
^ permalink raw reply
* Re: gitweb.cgi
From: Linus Torvalds @ 2005-10-19 0:27 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Kay Sievers, Git Mailing List
In-Reply-To: <43552FC2.3000000@zytor.com>
On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> I set up mod_cache (which I didn't know about, silly me) and so far it seems
> to work and has produced a tremendous decrease in load and improvement in
> response time.
It really doesn't work very well with the "front page" though..
Doing a "Save page as.." shows that it's not a huge page: it's roughly 700
lines long and 57kB in size, but pressing the reload button (or just going
somewhere else and coming back immediately) takes 45 seconds to reload for
me.
Trying again shows that it _is_ cached if you press the reload button
immediately again, but I haven't quite figured out how long the cache
timeout is. It seems to be around one minute (from some very preliminary
tests it's more than 25 seconds, but less than a minute and a half).
Considering that apparently the load is enough that it takes 45 seconds to
generate (scary in itself), is should clearly be cached for more than one
minute. More like ten minutes or half an hour, especially since mirroring
any content changes takes longer than that anyway.
Now, I suspect all the content on kernel.org could easily be cached for
ten minutes.
As far as I can tell, mod_cache without any expiry information uses
CacheDefaultExpire
which should default to one hour according to the docs. Have you changed
that to one minute? Maybe making it 10 minutes would be better?
That said, I tried to figure out how the front page is generated, but
haven't quite. Can somebody (Kay?) please say what it does most, and I can
try to make sure git does that efficiently..
Linus
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: H. Peter Anvin @ 2005-10-19 0:21 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0510181517280.3369@g5.osdl.org>
Linus Torvalds wrote:
>
> Hmm. The "not ending in /" is a bad test.
>
> Especially in light of the fact that the git-pack protocol quite by design
> tends to add a ".git" to the end as a fallback, so that a user that wants
> to specify a particular directory _without_ that fallback needs to have
> the slash at the end.
>
> Now, git-daemon hasn't implemented that, but I think that was just a
> mistake that grew out of it not getting a lot of testing, since it wasn't
> used much. I personally use the "without the final .git" version quite
> often, because it just looks so much nicer for the user.
>
> In fact, here's a patch that makes git-daemon allow it, and thus match the
> behaviour of the ssh transport.
>
> The logic is simple: if the original "chdir()" fails, try another one with
> ".git" appended. This is in _addition_ to doing the 'chdir(".git")' later,
> so that if you have a checked-out git repository in /home/linux-2.6.git,
> then doing a
>
This is also exactly the kind of DWIM that tends to result in the kind
of security holes I described earlier.
The DWIM aspect is fine, of course, but it has to be done up front:
instead of doing just chdir(), each path should be validated through
path_ok() before even being considered for chdir(). Perhaps the right
thing to do is to combine the two functions.
-hpa
^ permalink raw reply
* [RFC] Timeouts on HTTP requests
From: Nick Hengeveld @ 2005-10-18 23:51 UTC (permalink / raw)
To: git
Our QA department today checked what would happen if the network connection
went away completely in the middle of an HTTP transfer. It looks as though
the answer is that git-http-fetch sits there forever waiting for CURL to
return something.
I'm thinking of taking advantage of CURL's capability of aborting a request
if the transfer rate drops below a threshold for a specified length of time
using a new pair of environment variables and/or config file settings:
GIT_HTTP_LOW_SPEED_LIMIT/http.lowspeedlimit
GIT_HTTP_LOW_SPEED_TIME/http.lowspeedtime
Does this make sense, and if so should there be defaults if nothing is
specified?
--
For a successful technology, reality must take precedence over public
relations, for nature cannot be fooled.
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: Linus Torvalds @ 2005-10-18 23:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v64ruo31i.fsf@assigned-by-dhcp.cox.net>
On Tue, 18 Oct 2005, Junio C Hamano wrote:
>
> Wouldn't having /home/linux-2.6/.git/ repository with
> /home/linux-2.6/ working tree be good enough for that? Instead
> of doing "find / -type d -name '*.git'" you could do "find /
> -type d -name .git" for automated tasks.
In this case, yes.
But In a mixed environment where you might have "bare" repositories, you
want to have "reponame.git" as the repository name.
So with the rule that (a) try to first append ".git" and (b) then, after a
successful chdir, try to go in one more level, you can handle both types,
without ever having to care whether it's checked-out or not.
And for secondary projects (where git isn't necessarily the primary source
control method), I actually use the "project.git" naming just to make it
obvious that this is the "gitified" version of the project.
For example, I keep both my private uemacs and pine source trees as git
repositories these days, and I have them under "~/src/uemacs.git/" and
"~/src/pine.git/" even though they are checked out and thus actually have
another ".git" inside of them.
Linus
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: Junio C Hamano @ 2005-10-18 22:47 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0510181517280.3369@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> git pull git://host/home/linux-2.6
>
> will on the remote end do:
>
> chdir("/home/linux-2.6") // fails with ENOENT
> chdir("/home/linux-2.6.git") // works
> chdir(".git") // works
>
> resulting in it ending up in /home/linux-2.6.git/.git, which is exactly
> correct, and where it wants to be.
>
> I personally find it a nice bit of usability enhancement. You can name
> your git repositories with a ".git" suffix (which can help all kinds of
> automated tasks - like autopacking), but you don't force your users to
> care.
Wouldn't having /home/linux-2.6/.git/ repository with
/home/linux-2.6/ working tree be good enough for that? Instead
of doing "find / -type d -name '*.git'" you could do "find /
-type d -name .git" for automated tasks.
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: Linus Torvalds @ 2005-10-18 22:25 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Git Mailing List
In-Reply-To: <435560F7.4080006@zytor.com>
On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> This patch adds some extra paranoia to the git-daemon filename test. In
> particular, it now rejects pathnames containing // or ending with /; it also
> adds a redundant test for pathname absoluteness (belts and suspenders.)
Hmm. The "not ending in /" is a bad test.
Especially in light of the fact that the git-pack protocol quite by design
tends to add a ".git" to the end as a fallback, so that a user that wants
to specify a particular directory _without_ that fallback needs to have
the slash at the end.
Now, git-daemon hasn't implemented that, but I think that was just a
mistake that grew out of it not getting a lot of testing, since it wasn't
used much. I personally use the "without the final .git" version quite
often, because it just looks so much nicer for the user.
In fact, here's a patch that makes git-daemon allow it, and thus match the
behaviour of the ssh transport.
The logic is simple: if the original "chdir()" fails, try another one with
".git" appended. This is in _addition_ to doing the 'chdir(".git")' later,
so that if you have a checked-out git repository in /home/linux-2.6.git,
then doing a
git pull git://host/home/linux-2.6
will on the remote end do:
chmod("/home/linux-2.6") // fails with ENOENT
chmod("/home/linux-2.6.git") // works
chmod(".git") // works
resulting in it ending up in /home/linux-2.6.git/.git, which is exactly
correct, and where it wants to be.
I personally find it a nice bit of usability enhancement. You can name
your git repositories with a ".git" suffix (which can help all kinds of
automated tasks - like autopacking), but you don't force your users to
care.
Linus
---
diff --git a/daemon.c b/daemon.c
index 11fa3ed..a488512 100644
--- a/daemon.c
+++ b/daemon.c
@@ -128,8 +128,13 @@ static int upload(char *dir, int dirlen)
}
if (chdir(dir) < 0) {
- logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
- return -1;
+ int err = errno;
+ strcpy(dir + dirlen, ".git");
+ if (err != ENOENT || chdir(dir) < 0) {
+ dir[dirlen] = 0;
+ logerror("Cannot chdir('%s'): %s", dir, strerror(err));
+ return -1;
+ }
}
chdir(".git");
@@ -164,7 +169,12 @@ static int execute(void)
static char line[1000];
int len;
- len = packet_read_line(0, line, sizeof(line));
+ /*
+ * Make sure that we leave room for an extra ".git" at
+ * the end of the line. Note that the packet interfaces
+ * already guarantee that there is an ending '\0'.
+ */
+ len = packet_read_line(0, line, sizeof(line)-4);
if (len && line[len-1] == '\n')
line[--len] = 0;
^ permalink raw reply related
* [PATCH] Revised - git-daemon extra paranoia
From: H. Peter Anvin @ 2005-10-18 22:13 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Junio C Hamano, git
In-Reply-To: <43557254.3010807@zytor.com>
[-- Attachment #1: Type: text/plain, Size: 318 bytes --]
This patch adds some extra paranoia to the git-daemon filename test. In
particular, it now rejects pathnames containing //; it also adds a
redundant test for pathname absoluteness (belts and suspenders.)
A single / at the end of the path is still permitted, however.
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 833 bytes --]
diff --git a/daemon.c b/daemon.c
--- a/daemon.c
+++ b/daemon.c
@@ -80,17 +80,29 @@ static int path_ok(const char *dir)
{
const char *p = dir;
char **pp;
- int sl = 1, ndot = 0;
+ int sl, ndot;
+
+ /* The pathname here should be an absolute path. */
+ if ( *p++ != '/' )
+ return 0;
+
+ sl = 1; ndot = 0;
for (;;) {
if ( *p == '.' ) {
ndot++;
- } else if ( *p == '/' || *p == '\0' ) {
+ } else if ( *p == '\0' ) {
+ /* Reject "." and ".." at the end of the path */
if ( sl && ndot > 0 && ndot < 3 )
- return 0; /* . or .. in path */
+ return 0;
+
+ /* Otherwise OK */
+ break;
+ } else if ( *p == '/' ) {
+ /* Refuse "", "." or ".." */
+ if ( sl && ndot < 3 )
+ return 0;
sl = 1;
- if ( *p == '\0' )
- break; /* End of string and all is good */
} else {
sl = ndot = 0;
}
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: H. Peter Anvin @ 2005-10-18 22:08 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Junio C Hamano, git
In-Reply-To: <4355691D.2010200@zytor.com>
H. Peter Anvin wrote:
>
> For security, avoiding aliases is highly desirable, and if they're
> useless the easiest way to do that is to reject. If aliases are
> required, which it sounds like it might be, then canonicalization needs
> to be applied.
>
> This may sound redundant, but a lot of avoiding security holes involves
> applying good practices up front, instead of reactively.
>
I thought I might want to add a bit of an explanation, just for the
purpose of illustration.
Right now, we use a whitelist for access control. Aliases are not a
problem, because they fail shut.
A year from now, someone decides that they want a "all but" feature, and
thus adds a blacklist on top of the whitelist. If aliases are
permitted, unless the blacklist logic is written very carefully, one
would then be able to get around the blacklist by using one of the
aliased paths.
Improper handling of aliases is probably second only to buffer overflows
and large-string DoS attacks when it comes to security vulnerabilities.
-hpa
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: H. Peter Anvin @ 2005-10-18 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vll0qploy.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
>
> I would understand rejecting /../, and perhaps /./, but why
> reject // in between or / at the end?
>
For security, avoiding aliases is highly desirable, and if they're
useless the easiest way to do that is to reject. If aliases are
required, which it sounds like it might be, then canonicalization needs
to be applied.
This may sound redundant, but a lot of avoiding security holes involves
applying good practices up front, instead of reactively.
Allowing a terminal slash should be reasonably easy, though.
-hpa
^ permalink raw reply
* Re: [PATCH] git-daemon extra paranoia
From: Junio C Hamano @ 2005-10-18 21:19 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: git
In-Reply-To: <435560F7.4080006@zytor.com>
"H. Peter Anvin" <hpa@zytor.com> writes:
> This patch adds some extra paranoia to the git-daemon filename test. In
> particular, it now rejects pathnames containing // or ending with /; it
> also adds a redundant test for pathname absoluteness (belts and suspenders.)
>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Extra paranoia about non-canonical pathnames
I would understand rejecting /../, and perhaps /./, but why
reject // in between or / at the end?
Especially, I think this part in daemon.c::upload():
if (!path_ok(dir)) {
logerror("Forbidden directory: %s\n", dir);
return -1;
}
if (chdir(dir) < 0) {
logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
return -1;
}
chdir(".git");
relies on the fact that you can say "/home/junio/git/" for me to
publish "/home/junio/git/.git/" repository, so I would suspect
that it is necessary to allow "ending with /" at least.
^ permalink raw reply
* [PATCH] git-daemon extra paranoia
From: H. Peter Anvin @ 2005-10-18 20:54 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 271 bytes --]
This patch adds some extra paranoia to the git-daemon filename test. In
particular, it now rejects pathnames containing // or ending with /; it
also adds a redundant test for pathname absoluteness (belts and suspenders.)
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1185 bytes --]
Extra paranoia about non-canonical pathnames
---
commit a22f643931e48a319a70af7e91f809648160ecbf
tree 9d6934089c2628253d0690efde3fa7f36a1a8861
parent 4aaa702794447d9b281dd22fe532fd61e02434e1
author Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700
committer Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700
daemon.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/daemon.c b/daemon.c
--- a/daemon.c
+++ b/daemon.c
@@ -80,17 +80,25 @@ static int path_ok(const char *dir)
{
const char *p = dir;
char **pp;
- int sl = 1, ndot = 0;
+ int sl, ndot;
+
+ /* The pathname here should be an absolute path. */
+ if ( *p++ != '/' )
+ return 0;
+
+ sl = 1; ndot = 0;
for (;;) {
if ( *p == '.' ) {
ndot++;
} else if ( *p == '/' || *p == '\0' ) {
- if ( sl && ndot > 0 && ndot < 3 )
- return 0; /* . or .. in path */
+ if ( sl && ndot < 3 ) /* Refuse "", "." or ".." */
+ return 0;
sl = 1;
+
+ /* If this was end of string, we passed all tests */
if ( *p == '\0' )
- break; /* End of string and all is good */
+ break;
} else {
sl = ndot = 0;
}
^ permalink raw reply
* Re: Hard-linked trees with git?
From: Junio C Hamano @ 2005-10-18 20:52 UTC (permalink / raw)
To: Krzysztof Halasa; +Cc: git
In-Reply-To: <m3vezufujo.fsf@defiant.localdomain>
Krzysztof Halasa <khc@pm.waw.pl> writes:
> Or: is it possible to have some constant file timestamps, so that
> changing the HEAD to something and returning to the old HEAD
> (say, with hard resets) restores the old timestamps?
That would screw up 'make'.
^ permalink raw reply
* Re: git-diff-tree rename detection for single file
From: Junio C Hamano @ 2005-10-18 20:50 UTC (permalink / raw)
To: David Ho; +Cc: git
In-Reply-To: <4dd15d180510181256i1c5a82d9ld62acaedb493cf71@mail.gmail.com>
David Ho <davidkwho@gmail.com> writes:
> I have a small suggestion to make the diff of a renamed file a bit
> more meaningful. I have a file that is renamed-edited and commited.
> git-diff-tree -M -p <commit> shows one result and git-diff-tree -M -p
> <commit> <filename> shows another. If they both show a rename
> occurred then I think the single file git-diff-tree will be more
> useful.
Sorry, this has vetoed by Linus long time ago. The <filename>
restricts the paths being passed to the diff machinery upfront,
so once you say <filename>, the rename detection will see only
that path and nothing else to compare and guess which other file
that file in question is a copy of.
^ permalink raw reply
* Re: [CORRECTED PATCH] git-fetch-pack: avoid unnecessary zero packing
From: Linus Torvalds @ 2005-10-18 20:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0510181333380.3369@g5.osdl.org>
On Tue, 18 Oct 2005, Linus Torvalds wrote:
>
> I'll see if I can come up with a good patch.
I see you already did. Looks fine. I'd suggest limiting the commits by
number in mark_recent_commit_complete(), because
(a) somebody might have their clock set wrong and you don't want to walk
a huge tree just because of something like that.
(b) you might just have imported a huge history (badly) from somewhere
else
(c) a _lot_ can happen in five days with automated things.
but yes, the approach looks very sane otherwise.
Linus
^ permalink raw reply
* Re: gitweb.cgi
From: Brian Gerst @ 2005-10-18 20:44 UTC (permalink / raw)
To: Kay Sievers; +Cc: H. Peter Anvin, Git Mailing List
In-Reply-To: <43552FC2.3000000@zytor.com>
H. Peter Anvin wrote:
> Kay Sievers wrote:
>
>>
>>> Most of the hits we get are either the gitweb front page or the
>>> gitweb rss feeds, and it's eating I/O bandwidth like crazy.
>>
>>
>> I tested some stuff on these boxes and 30 stat() calls alone take app.
>> 2 seconds
>> on these boxes cause of I/O load ... :)
>>
>
> Welcome to my hell :)
>
> I set up mod_cache (which I didn't know about, silly me) and so far it
> seems to work and has produced a tremendous decrease in load and
> improvement in response time. I do, have, however, a request. There
> are some gitweb pages which are more likely to change than others; in
> particular, some gitweb pages will *never* change (because they directly
> reflect immutable git data.)
>
> If gitweb could produce Last-Modified and Expires headers where
> appropriate, it should improve caching performance.
>
> -hpa
Some other areas for improvement would be to seperate out the git icon
and the style sheet into seperate static files.
--
Brian Gerst
^ permalink raw reply
* Re: [CORRECTED PATCH] git-fetch-pack: avoid unnecessary zero packing
From: Linus Torvalds @ 2005-10-18 20:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vmzl6r78e.fsf@assigned-by-dhcp.cox.net>
On Tue, 18 Oct 2005, Junio C Hamano wrote:
>
> It strikes me that we could walk from our refs, depth reasonably
> limited to say 20 or so commit chain and/or last 5 days of
> commit time, to see if any of the remotes are reachable from our
> refs and omit issuing "want" quite cheaply. Do you think that
> would be a worthy change to make things more efficient?
Probably doesn't make a huge difference, but it might be worth trying.
There's a cheap test you can do _before_ you even start walking: check if
you have the object that is pointed to by the remote ref at all. If you
don't have it, then you know it can't be reachable from any of the local
refs. And if you do have it, the likelihood that it _is_ reachable is
likely pretty high.
(I didn't do that for the current fetch-pack optimization, since just
doing the read_ref() is likely faster than even bothering with the object
lookup. But if you start traversing commit lists, it suddenly becomes
more worthwhile).
You'd need to look up the object anyway in order to figure out that it's a
commit (or points to a commit).
So it might be wasting a bit of time, but the good news is that it wastes
time on the _client_ side, where we've got plenty.
I'll see if I can come up with a good patch.
Linus
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox