threads / patch / 14548

patchgit-shell needs $(COMPAT_OBJS)

Subject: [PATCH] git-shell needs $(COMPAT_OBJS)

## tl;dr

8 messages between Jul 20, 2008 and Jul 20, 2008. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Johannes Sixt· Jul 20, 2008, 19:11 UTC · lore
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
  Discovered by an accidental NO_MMAP=1 in config.mak.
  -- Hannes
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index c676d97..2be40eb 100644
--- a/Makefile
+++ b/Makefile
@@ -1203,7 +1203,7 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
-git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o
+git-shell$X: abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
 
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
-- 
1.5.6.3.443.g5f70e
Johannes Sixt· Jul 20, 2008, 21:34 UTC · re: Johannes Sixt · lore

Re: [PATCH] git-shell needs $(COMPAT_OBJS)

On Sonntag, 20. Juli 2008, Johannes Sixt wrote:
> -git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o
> strbuf.o usage.o wrapper.o shell.o
> +git-shell$X: abspath.o ctype.o 
> exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)

Unfortunately, that's only half the deal. If we compile with NO_PREAD=1, this needs read_in_full(), which is in write_or_die.o. I don't know how to fix this without insulting your good taste except for reverting the recent commits that touch this line...

-- Hannes
Junio C Hamano· Jul 20, 2008, 22:15 UTC · re: Johannes Sixt · lore

Re: [PATCH] git-shell needs $(COMPAT_OBJS)

Johannes Sixt <johannes.sixt@telecom.at> writes:
Show 8 quoted lines
> On Sonntag, 20. Juli 2008, Johannes Sixt wrote:
>> -git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o
>> strbuf.o usage.o wrapper.o shell.o
>> +git-shell$X: abspath.o ctype.o 
>> exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)
>
> Unfortunately, that's only half the deal. If we compile with NO_PREAD=1, this 
> needs read_in_full(),...

Well, if compat/* implementations use anything outside compat/ left and right, then all bets are off.

Why do we care about the size of git-shell so much in the first place anyway to begin with?

Johannes Schindelin· Jul 20, 2008, 22:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-shell needs $(COMPAT_OBJS)

Hi,
On Sun, 20 Jul 2008, Junio C Hamano wrote:
> Why do we care about the size of git-shell so much in the first place 
> anyway to begin with?

It was not me who proposed it, but I guess it was for auditing purposes: git-shell is often the only point of entry for certain untrusted ssh users, and the less code is linked, the less code has to be analyzed for reachability (and then for security holes).

Ciao, Dscho

Junio C Hamano· Jul 20, 2008, 22:38 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-shell needs $(COMPAT_OBJS)

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
> On Sun, 20 Jul 2008, Junio C Hamano wrote:
>
>> Why do we care about the size of git-shell so much in the first place 
>> anyway to begin with?
>
> It was not me who proposed it, but I guess it was for auditing purposes: 
> git-shell is often the only point of entry for certain untrusted ssh 
> users, and the less code is linked, the less code has to be analyzed for 
> reachability (and then for security holes).
That's a rather misguided approach, isn't it?

After all, the work requested by the end user will be handled by code in the main git executable by spawning a subprocess, and you are auditing the codepath that leads to the spawning anyway.

Miklos Vajna· Jul 20, 2008, 23:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-shell needs $(COMPAT_OBJS)

On Sun, Jul 20, 2008 at 03:38:53PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> That's a rather misguided approach, isn't it?
> 
> After all, the work requested by the end user will be handled by code in
> the main git executable by spawning a subprocess, and you are auditing the
> codepath that leads to the spawning anyway.
Hm, but then what's the purpose of having git-shell as a not-builtin?
Junio C Hamano· Jul 20, 2008, 22:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-shell needs $(COMPAT_OBJS)

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Johannes Sixt <johannes.sixt@telecom.at> writes:
>
>> On Sonntag, 20. Juli 2008, Johannes Sixt wrote:
>>> -git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o
>>> strbuf.o usage.o wrapper.o shell.o
>>> +git-shell$X: abspath.o ctype.o 
>>> exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_OBJS)
>>
>> Unfortunately, that's only half the deal. If we compile with NO_PREAD=1, this 
>> needs read_in_full(),...
>
> Well, if compat/* implementations use anything outside compat/ left and
> right, then all bets are off.

If compat/ layer tries to really be about "compatibility", they should not be using things like xmalloc() that call release_pack_meory() that is in sha1_file.c. From a quick glance, mingw.c, mmap.c, pread.c are major offenders, but others such as setenv.c seem to be carefully written.

Perhaps we should apply a variant of your patch to allow linking from compat/ routines to git-shell, so that people affected by the compat layer functions that call outside compat layer have incentive to fix them.

 Makefile |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
Show changes to Makefile +7 −3
diff --git a/Makefile b/Makefile
index 2b670d7..551bde9 100644
--- a/Makefile
+++ b/Makefile
@@ -324,6 +324,7 @@ endif
 export PERL_PATH
 
 LIB_FILE=libgit.a
+COMPAT_LIB = compat/lib.a
 XDIFF_LIB=xdiff/lib.a
 
 LIB_H += archive.h
@@ -1203,8 +1204,11 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
-git-shell$X: compat/strlcpy.o abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
+$(COMPAT_LIB): $(COMPAT_OBJS)
+	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(COMPAT_OBJS)
+
+git-shell$X: abspath.o ctype.o exec_cmd.o quote.o strbuf.o usage.o wrapper.o shell.o $(COMPAT_LIB)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(COMPAT_LIB)
 
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
@@ -1402,7 +1406,7 @@ distclean: clean
 
 clean:
 	$(RM) *.o mozilla-sha1/*.o arm/*.o ppc/*.o compat/*.o xdiff/*.o \
-		$(LIB_FILE) $(XDIFF_LIB)
+		$(LIB_FILE) $(XDIFF_LIB) $(COMPAT_LIB)
 	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
Junio C Hamano· Jul 20, 2008, 23:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-shell needs $(COMPAT_OBJS)

Junio C Hamano <gitster@pobox.com> writes:
> Perhaps we should apply a variant of your patch to allow linking from
> compat/ routines to git-shell, so that people affected by the compat layer
> functions that call outside compat layer have incentive to fix them.
So the previous one will be queued in 'pu' as...
    Link shell with compat layer functions
    
    This in the short term will break on platforms that use compat implemenations
    that call outside compat layer, but that is exactly what we want.  To give
    incentive to fix things for people who are affected and more importantly have
    environment to test their fixes.
    
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

and here is an example to fix pread for you, even I do not need it, though.

-- >8 -- Move read_in_full() and write_in_full() to wrapper.c

A few compat/* layer functions call these functions, but we would really want to keep them thin, without depending too much on the libgit proper.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wrapper.c      |   38 ++++++++++++++++++++++++++++++++++++++
 write_or_die.c |   38 --------------------------------------
 2 files changed, 38 insertions(+), 38 deletions(-)
Show changes to 2 files +38 −38

wrapper.c, write_or_die.c

diff --git a/wrapper.c b/wrapper.c
index 4e04f76..93562f0 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -133,6 +133,44 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
+ssize_t read_in_full(int fd, void *buf, size_t count)
+{
+	char *p = buf;
+	ssize_t total = 0;
+
+	while (count > 0) {
+		ssize_t loaded = xread(fd, p, count);
+		if (loaded <= 0)
+			return total ? total : loaded;
+		count -= loaded;
+		p += loaded;
+		total += loaded;
+	}
+
+	return total;
+}
+
+ssize_t write_in_full(int fd, const void *buf, size_t count)
+{
+	const char *p = buf;
+	ssize_t total = 0;
+
+	while (count > 0) {
+		ssize_t written = xwrite(fd, p, count);
+		if (written < 0)
+			return -1;
+		if (!written) {
+			errno = ENOSPC;
+			return -1;
+		}
+		count -= written;
+		p += written;
+		total += written;
+	}
+
+	return total;
+}
+
 int xdup(int fd)
 {
 	int ret = dup(fd);
diff --git a/write_or_die.c b/write_or_die.c
index e4c8e22..4c29255 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -45,44 +45,6 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 	}
 }
 
-ssize_t read_in_full(int fd, void *buf, size_t count)
-{
-	char *p = buf;
-	ssize_t total = 0;
-
-	while (count > 0) {
-		ssize_t loaded = xread(fd, p, count);
-		if (loaded <= 0)
-			return total ? total : loaded;
-		count -= loaded;
-		p += loaded;
-		total += loaded;
-	}
-
-	return total;
-}
-
-ssize_t write_in_full(int fd, const void *buf, size_t count)
-{
-	const char *p = buf;
-	ssize_t total = 0;
-
-	while (count > 0) {
-		ssize_t written = xwrite(fd, p, count);
-		if (written < 0)
-			return -1;
-		if (!written) {
-			errno = ENOSPC;
-			return -1;
-		}
-		count -= written;
-		p += written;
-		total += written;
-	}
-
-	return total;
-}
-
 void fsync_or_die(int fd, const char *msg)
 {
 	if (fsync(fd) < 0) {

← back to recent threads