{"thread":{"id":"14548","subject":"[PATCH] git-shell needs $(COMPAT_OBJS)","startedAt":"2008-07-20T19:11:48Z","lastAt":"2008-07-20T23:12:18Z","messageCount":8,"participants":["Johannes Sixt","Junio C Hamano","Johannes Schindelin","Miklos Vajna"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"84053","messageId":"200807202111.48332.johannes.sixt@telecom.at","threadId":"14548","inReplyTo":null,"subject":"[PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-07-20T19:11:48Z","receivedAt":"2008-07-20T19:11:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt \n---\n Discovered by an accidental NO_MMAP=1 in config.mak.\n\n -- Hannes\n\n Makefile | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c676d97..2be40eb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1203,7 +1203,7 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n \t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n \n-git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o\n+git-shell$X: abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)\n \n $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)\n-- \n1.5.6.3.443.g5f70e\n"},{"id":"84072","messageId":"200807202334.36506.johannes.sixt@telecom.at","threadId":"14548","inReplyTo":"200807202111.48332.johannes.sixt@telecom.at","subject":"Re: [PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-07-20T21:34:36Z","receivedAt":"2008-07-20T21:34:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 20. Juli 2008, Johannes Sixt wrote:\n> -git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o\n> strbuf.o usage.o wrapper.o shell.o\n> +git-shell$X: abspath.o ctype.o \n> exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)\n\nUnfortunately, that's only half the deal. If we compile with NO_PREAD=1, this \nneeds read_in_full(), which is in write_or_die.o. I don't know how to fix \nthis without insulting your good taste except for reverting the recent \ncommits that touch this line...\n\n-- Hannes\n"},{"id":"84081","messageId":"7vzloc2odx.fsf@gitster.siamese.dyndns.org","threadId":"14548","inReplyTo":"200807202334.36506.johannes.sixt@telecom.at","subject":"Re: [PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-20T22:15:38Z","receivedAt":"2008-07-20T22:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> On Sonntag, 20. Juli 2008, Johannes Sixt wrote:\n>> -git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o\n>> strbuf.o usage.o wrapper.o shell.o\n>> +git-shell$X: abspath.o ctype.o \n>> exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)\n>\n> Unfortunately, that's only half the deal. If we compile with NO_PREAD=1, this \n> needs read_in_full(),...\n\nWell, if compat/* implementations use anything outside compat/ left and\nright, then all bets are off.\n\nWhy do we care about the size of git-shell so much in the first place\nanyway to begin with?\n"},{"id":"84085","messageId":"alpine.DEB.1.00.0807210034050.3305@eeepc-johanness","threadId":"14548","inReplyTo":"7vzloc2odx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-20T22:35:18Z","receivedAt":"2008-07-20T22:35:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 20 Jul 2008, Junio C Hamano wrote:\n\n> Why do we care about the size of git-shell so much in the first place \n> anyway to begin with?\n\nIt was not me who proposed it, but I guess it was for auditing purposes: \ngit-shell is often the only point of entry for certain untrusted ssh \nusers, and the less code is linked, the less code has to be analyzed for \nreachability (and then for security holes).\n\nCiao,\nDscho\n"},{"id":"84087","messageId":"7vprp82nb6.fsf@gitster.siamese.dyndns.org","threadId":"14548","inReplyTo":"alpine.DEB.1.00.0807210034050.3305@eeepc-johanness","subject":"Re: [PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-20T22:38:53Z","receivedAt":"2008-07-20T22:38:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sun, 20 Jul 2008, Junio C Hamano wrote:\n>\n>> Why do we care about the size of git-shell so much in the first place \n>> anyway to begin with?\n>\n> It was not me who proposed it, but I guess it was for auditing purposes: \n> git-shell is often the only point of entry for certain untrusted ssh \n> users, and the less code is linked, the less code has to be analyzed for \n> reachability (and then for security holes).\n\nThat's a rather misguided approach, isn't it?\n\nAfter all, the work requested by the end user will be handled by code in\nthe main git executable by spawning a subprocess, and you are auditing the\ncodepath that leads to the spawning anyway.\n"},{"id":"84091","messageId":"7vhcak2mip.fsf@gitster.siamese.dyndns.org","threadId":"14548","inReplyTo":"7vzloc2odx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-20T22:55:58Z","receivedAt":"2008-07-20T22:55:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Sixt writes:\n>\n>> On Sonntag, 20. Juli 2008, Johannes Sixt wrote:\n>>> -git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o\n>>> strbuf.o usage.o wrapper.o shell.o\n>>> +git-shell$X: abspath.o ctype.o \n>>> exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)\n>>\n>> Unfortunately, that's only half the deal. If we compile with NO_PREAD=1, this \n>> needs read_in_full(),...\n>\n> Well, if compat/* implementations use anything outside compat/ left and\n> right, then all bets are off.\n\nIf compat/ layer tries to really be about \"compatibility\", they should not\nbe using things like xmalloc() that call release_pack_meory() that is in\nsha1_file.c. From a quick glance, mingw.c, mmap.c, pread.c are major\noffenders, but others such as setenv.c seem to be carefully written.\n\nPerhaps we should apply a variant of your patch to allow linking from\ncompat/ routines to git-shell, so that people affected by the compat layer\nfunctions that call outside compat layer have incentive to fix them.\n\n Makefile | 10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 2b670d7..551bde9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -324,6 +324,7 @@ endif\n export PERL_PATH\n \n LIB_FILE=libgit.a\n+COMPAT_LIB = compat/lib.a\n XDIFF_LIB=xdiff/lib.a\n \n LIB_H += archive.h\n@@ -1203,8 +1204,11 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n \t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n \n-git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o\n-\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)\n+$(COMPAT_LIB): $(COMPAT_OBJS)\n+\t$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(COMPAT_OBJS)\n+\n+git-shell$X: abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_LIB)\n+\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(COMPAT_LIB)\n \n $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)\n $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)\n@@ -1402,7 +1406,7 @@ distclean: clean\n \n clean:\n \t$(RM) *.o mozilla-sha1/*.o arm/*.o ppc/*.o compat/*.o xdiff/*.o \\\n-\t\t$(LIB_FILE) $(XDIFF_LIB)\n+\t\t$(LIB_FILE) $(XDIFF_LIB) $(COMPAT_LIB)\n \t$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X\n \t$(RM) $(TEST_PROGRAMS)\n \t$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*\n"},{"id":"84096","messageId":"20080720230833.GT10347@genesis.frugalware.org","threadId":"14548","inReplyTo":"7vprp82nb6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-20T23:08:33Z","receivedAt":"2008-07-20T23:08:33Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sun, Jul 20, 2008 at 03:38:53PM -0700, Junio C Hamano wrote:\n> That's a rather misguided approach, isn't it?\n> \n> After all, the work requested by the end user will be handled by code in\n> the main git executable by spawning a subprocess, and you are auditing the\n> codepath that leads to the spawning anyway.\n\nHm, but then what's the purpose of having git-shell as a not-builtin?\n"},{"id":"84098","messageId":"7v7ibg2lrh.fsf@gitster.siamese.dyndns.org","threadId":"14548","inReplyTo":"7vhcak2mip.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-shell needs $(COMPAT_OBJS)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-20T23:12:18Z","receivedAt":"2008-07-20T23:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps we should apply a variant of your patch to allow linking from\n> compat/ routines to git-shell, so that people affected by the compat layer\n> functions that call outside compat layer have incentive to fix them.\n\nSo the previous one will be queued in 'pu' as...\n\n Link shell with compat layer functions\n \n This in the short term will break on platforms that use compat implemenations\n that call outside compat layer, but that is exactly what we want. To give\n incentive to fix things for people who are affected and more importantly have\n environment to test their fixes.\n \n Signed-off-by: Junio C Hamano \n\nand here is an example to fix pread for you, even I do not need it,\nthough.\n\n-- >8 --\nMove read_in_full() and write_in_full() to wrapper.c\n\nA few compat/* layer functions call these functions, but we would really\nwant to keep them thin, without depending too much on the libgit proper.\n\nSigned-off-by: Junio C Hamano \n---\n wrapper.c | 38 ++++++++++++++++++++++++++++++++++++++\n write_or_die.c | 38 --------------------------------------\n 2 files changed, 38 insertions(+), 38 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 4e04f76..93562f0 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -133,6 +133,44 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n+ssize_t read_in_full(int fd, void *buf, size_t count)\n+{\n+\tchar *p = buf;\n+\tssize_t total = 0;\n+\n+\twhile (count > 0) {\n+\t\tssize_t loaded = xread(fd, p, count);\n+\t\tif (loaded <= 0)\n+\t\t\treturn total ? total : loaded;\n+\t\tcount -= loaded;\n+\t\tp += loaded;\n+\t\ttotal += loaded;\n+\t}\n+\n+\treturn total;\n+}\n+\n+ssize_t write_in_full(int fd, const void *buf, size_t count)\n+{\n+\tconst char *p = buf;\n+\tssize_t total = 0;\n+\n+\twhile (count > 0) {\n+\t\tssize_t written = xwrite(fd, p, count);\n+\t\tif (written < 0)\n+\t\t\treturn -1;\n+\t\tif (!written) {\n+\t\t\terrno = ENOSPC;\n+\t\t\treturn -1;\n+\t\t}\n+\t\tcount -= written;\n+\t\tp += written;\n+\t\ttotal += written;\n+\t}\n+\n+\treturn total;\n+}\n+\n int xdup(int fd)\n {\n \tint ret = dup(fd);\ndiff --git a/write_or_die.c b/write_or_die.c\nindex e4c8e22..4c29255 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -45,44 +45,6 @@ void maybe_flush_or_die(FILE *f, const char *desc)\n \t}\n }\n \n-ssize_t read_in_full(int fd, void *buf, size_t count)\n-{\n-\tchar *p = buf;\n-\tssize_t total = 0;\n-\n-\twhile (count > 0) {\n-\t\tssize_t loaded = xread(fd, p, count);\n-\t\tif (loaded <= 0)\n-\t\t\treturn total ? total : loaded;\n-\t\tcount -= loaded;\n-\t\tp += loaded;\n-\t\ttotal += loaded;\n-\t}\n-\n-\treturn total;\n-}\n-\n-ssize_t write_in_full(int fd, const void *buf, size_t count)\n-{\n-\tconst char *p = buf;\n-\tssize_t total = 0;\n-\n-\twhile (count > 0) {\n-\t\tssize_t written = xwrite(fd, p, count);\n-\t\tif (written < 0)\n-\t\t\treturn -1;\n-\t\tif (!written) {\n-\t\t\terrno = ENOSPC;\n-\t\t\treturn -1;\n-\t\t}\n-\t\tcount -= written;\n-\t\tp += written;\n-\t\ttotal += written;\n-\t}\n-\n-\treturn total;\n-}\n-\n void fsync_or_die(int fd, const char *msg)\n {\n \tif (fsync(fd) < 0) {\n"}]}