[PATCH] git-shell needs $(COMPAT_OBJS)

[PATCH] git-shell needs $(COMPAT_OBJS)

[Johannes Sixt]

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(-)

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

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

[Johannes Sixt]

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

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

[Junio C Hamano]

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.

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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.

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

On Sun, Jul 20, 2008 at 03:38:53PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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(-)

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*

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

[Junio C Hamano]

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(-)

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) {