gmane.comp.java.classpath.devel

array overflow in local.c

Hi,
gcc found a problem in our native/jni/javanet/local.c.

I changed it to what I think makes sense but I am not sure whether this
is still the intended behavior.

Furthermore since overrunning the bounds of a stack allocated array may
trash other stuff on the stack I wonder whether this fix also prevents
the problem that the workaround above the modified code speaks of. Since
I do not run Darwin-based OS I cannot test it myself.

Regards
Robert

Index: classpath-0.96.1/native/jni/java-net/local.c
===================================================================
--- classpath-0.96.1.orig/native/jni/java-net/local.c	2008-06-27 11:21:31.000000000 +0200
+++ classpath-0.96.1/native/jni/java-net/local.c	2008-06-27 11:21:41.000000000 +0200
@@ -93,7 +93,7 @@
     }

   strncpy (saddr.sun_path, addr, sizeof (saddr.sun_path));
-  saddr.sun_path[sizeof (saddr.sun_path)] = '\0';
+  saddr.sun_path[sizeof (saddr.sun_path) - 1] = '\0';
   saddr.sun_family = AF_LOCAL;

   return bind (fd, (struct sockaddr *) &saddr, SUN_LEN (&saddr));

headers

Andrew Haley | 27 Jun 12:22

Re: array overflow in local.c

Robert Schuster wrote:

> gcc found a problem in our native/jni/javanet/local.c.
> 
> I changed it to what I think makes sense but I am not sure whether this
> is still the intended behavior.
> 
> Furthermore since overrunning the bounds of a stack allocated array may
> trash other stuff on the stack I wonder whether this fix also prevents
> the problem that the workaround above the modified code speaks of. Since
> I do not run Darwin-based OS I cannot test it myself.

That may well be right.

IMO it should be more like

Index: local.c
===================================================================
RCS file: /cvsroot/classpath/classpath/native/jni/java-net/local.c,v
retrieving revision 1.4
diff -u -r1.4 local.c
--- local.c	17 Apr 2007 21:46:27 -0000	1.4
+++ local.c	27 Jun 2008 10:21:12 -0000
@@ -86,14 +86,13 @@
   if (gcc_sucks)
     fprintf (stderr, "bind %p\n", addr);

-  if (strlen (addr) > sizeof (saddr.sun_path))
+  if (strlen (addr) >= sizeof (saddr.sun_path))
     {

(Continue reading)

headers

Robert Schuster | 27 Jun 15:15

Re: array overflow in local.c

Hi.

Andrew Haley schrieb:
>> Furthermore since overrunning the bounds of a stack allocated array may
>> trash other stuff on the stack I wonder whether this fix also prevents
>> the problem that the workaround above the modified code speaks of. Since
>> I do not run Darwin-based OS I cannot test it myself.
> 
> That may well be right.
> 
> IMO it should be more like
OK.

Casey would you mind testing the attached patch on your Darwin platform?

Regards
Robert

Index: native/jni/java-net/local.c
===================================================================
RCS file: /sources/classpath/classpath/native/jni/java-net/local.c,v
retrieving revision 1.4
diff -u -r1.4 local.c
--- native/jni/java-net/local.c	17 Apr 2007 21:46:27 -0000	1.4
+++ native/jni/java-net/local.c	27 Jun 2008 13:14:40 -0000
@@ -73,27 +73,18 @@
   return socket (PF_UNIX, stream ? SOCK_STREAM : SOCK_DGRAM, 0);
 }

(Continue reading)

headers

Casey Marshall | 27 Jun 19:22

Re: array overflow in local.c

On Jun 27, 2008, at 6:15 AM, Robert Schuster wrote:

> Hi.
>
> Andrew Haley schrieb:
>>> Furthermore since overrunning the bounds of a stack allocated  
>>> array may
>>> trash other stuff on the stack I wonder whether this fix also  
>>> prevents
>>> the problem that the workaround above the modified code speaks of.  
>>> Since
>>> I do not run Darwin-based OS I cannot test it myself.
>>
>> That may well be right.
>>
>> IMO it should be more like
> OK.
>
> Casey would you mind testing the attached patch on your Darwin  
> platform?
>

I'll give it a try when I get a chance, but this patch looks fine as is.

(And I'm embarrassed; `addr' was being set to NULL, if I remember  
correctly)