Created attachment 507 [details] [review]
Diff file for testglib

Created attachment 508 [details] [review]
Diff file for gstrfuncs.c.  Modifies g_strlcat to check size.

Running on Redhat Linux 7.1 Kernel 2.4.2-2 i686.
C library version:  libc.so.6


When compiling glib on Solaris 8 I found that it core dumped due to an
assertion failing at line 1017.  The assertion was testing g_strlcat
for a return value of 3, but on Solaris it was returning 4.  Also,
according to the strlcat documentation on Solaris this is the correct
value.


Check on Linux and found no problem.  Linux uses the glib
implementation of g_strlcat.  This is based on OpenBSD so I check the
OpenBSD man pages at:
 http://www.openbsd.org/cgi-bin/man.cgi?query=strlcat&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html

The return values are specified as follows:

>RETURN VALUES
>The strlcpy() and strlcat() functions return the total length of the
>string they tried to create.  For strlcpy() that means the length of
>src.
>For strlcat() that means the initial length of dst plus the length of
>src. While this may seem somewhat confusing it was done to make
>truncation detection simple.

According to this, g_strlcat should indeed return 4 for the testcase
used in testglib.c

g_strlcat can remedy this by checking the third parameter, size, to
see if it is 0 before any processing.  In that case it simply returns
the length of the two strings passed in as parameters.

Two diff files are attached with changes for g_strlcat and the tests
in testglib.c.

Also attached is a small test file to exercise this portion of g_strlcat.

Created attachment 510 [details]
Simple testcase to exercise g_strlcat with parameter 3 = 0

Hmm, reading the OpenBSD man page doesn't exactly make things
clear to me. I think essentially, the case where 
strlen(dest) >= size is undefined.

So, my instinct here is to simply remove the test case.

The other approach would be to find an OpenBSD system and
see what it returns; I don't want to simply change the check
and have it break on OpenBSD....

Move some forgotten unclosed bugs from earlier 2.0.x milestones to 2.0.10

Looking at testglib.c, there are some more strlcat tests which 
seem to assert the 
wrong return value, e.g.

  *string = 'Y';
  *(string + 1)= '\0';
  g_assert 
(g_strlcpy(string, "Hello" , 0) == 5);

which should return initial length of 
dest + length of src == 6.

Why doesn't that assert trigger on Solaris 8 ?

Hmm, I believe there is a indeed a problem with g_strcat. It is even
documented to behave differently from what the OpenBSD man page says.
In gstrfuncs.c I find:

  Returns size of attempted result, which is
  MIN (dest_size, strlen (original dest)) + strlen (src),
  so if retval >= dest_size, truncation occurred.

I think we must either fix our implementation and tests to follow the
OpenBSD man page or always use our implementation and never wrap a
system strlcat, since it will probably have the OpenBSD semantics.

I vote for the first alternative.

As always, I spoke too soon...

Reading the complete section of the OpenBSD man page about return
values reveals that:

     Note however, that if strlcat() traverses size characters 
     without finding a NUL, the length of the string is considered 
     to be size and the destination string will not be NUL-terminated
     (since there was no space for the NUL).  This keeps strlcat()
     from running off the end of a string.  In practice this should
     not happen (as it means that either size is incorrect or that 
     dst is not a proper ``C'' string).  The check exists to 
     prevent potential security problems in incorrect code.

This is exactly what g_strlcat implements and what the tests check.
I conclude that the Solaris 8 implementation of strlcat fails to 
implement this paragraph - maybe we should put some of the testglib.c
tests into a configure test to check for a good strlcat before
wrapping it ?
Another idea is to issue a warning if the case above is met (since
it means a programming error: either the size is incorrect or the 
dst is not a proper C string)

Here is what David Wheeler, the auther of g_strlcpy and g_strlcat has
to say on the issue (found in 
http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO)

Also, strlcat(3) has slightly varying semantics when the provided size
is 0 or if there are no NIL characters in the destination string dst
(inside the given number of characters). In OpenBSD, if the size is 0,
then the destination string's length is considered 0. Also, if size is
nonzero, but there are no NIL characters in the destination string (in
the size number of characters), then the length of the destination is
considered equal to the size. These rules make handling strings
without embedded NILs consistent. Unfortunately, at least Solaris
doesn't (at this time) obey these rules, because they weren't
specified in the original documentation. I've talked to Todd Miller,
and he and I agree that the OpenBSD semantics are the correct ones
(and that Solaris is incorrect). The reasoning is simple: under no
condition should strlcat or strlcpy ever examine characters in the
destination outside of the range of size; such access might cause core
dumps (from accessing out-of-range memory) and even hardware
interactions (through memory-mapped I/O). Thus, given:

  a = strlcat ("Y", "123", 0);

The correct answer is 3 (0+3=3), but Solaris will claim the answer is
4 because it incorrectly looks at characters beyond the "size" length
in the destination. For now, I suggest avoiding cases where the size
is 0 or the destination has no NIL characters. Future versions of glib
will hide this difference and always use the OpenBSD semantics.

Curious that he talks about future versions of glib, but I think he
points in the direction I mentioned earlier: we should use

strlcat ("Y", "123", 0) == 3 

as autoconf test for a strlcat with the OpenBSD semantics 

Here is a configure.in patch (which I have not tested on either
OpenBSD or Solaris 8). 

Created attachment 12581 [details] [review]
patch

Looks, basically OK, but it seems to me that the check
could be simplified to.

+int main() {
+  char p[10] = "hi";
+  if (strlcat (p, "bye", 0) != 3) 
+    return 1;
+  return 0;
+}], glib_ok=yes, glib_ok=no)

with your current version, you seem to be missing a stdlib.h
include. Also, AC_TRY_RUN() is a bit of a problem for cross
compilation, so:

 - You should provide a false (glib_ok=no makes a lot of sense)
 - You probably should enclose the check in a AC_CACHE_CHECK

   AC_CACHE_CHECK([for a working strlcat()], [glib_cv_have_strlcpy],
     [AC_TRY_RUN (..., 
                  glib_cv_have_strlcpy=yes, 
                  glib_cv_have_strlcpy=no,
                  glib_cv_have_strlcpy=no)])

   Bug 77565 is full of patches to fix cross compilation, and
   I don't want to _regress_ here.