Patch to fix Reference/natReference.

[David Daney <ddaney at avtrex dot com>]

Finally found it!  The attached test program exercises a bug in libgcj's
lava.lang.ref.Reference handling code (from gcc 3.3.1).  It crashes on 
both my i686-pc-linux-gnu and mipsel-linux platforms

If Reference.clear() is called and then the Reference is finalized
before its referent, a dangling pointer is created in the object_list
structure in natReference.cc.  This happens because the 'copy' field
of the Reference is cleared and that is what is used to find the slot
in the object_list table.

When more objects are allocated, this dangling pointer now can point
to an object of some other type.

Now when the original referent is finalized, all of the References to
it are enqueued, but since some of the pointers to these References
are invalid, the called to enqueue() (via a now invalid vtable) sends
us off into some place bad.

There seem to be two ways to fix the problem.

1) Never clear the 'copy' field in the Reference and add a boolean
    field to Reference that is set to detect the cleared condition.
    Doing this we will always be able to find the Reference in the
    object_list.

2) If 'copy' is null, then scan the entire object_list tree looking
   for the entry that points to the Reference and remove it.


I have attached a patch that does #1.

Question: Does adding a field to Reference cause binary compatibility
problems between different versions of shared libgcj?

I don't think so because all fields are package private.

Option 2 preserves the size and layout of Reference, but would be
slower and require bigger changes to natReference.cc

My patch includes changes from Tom Tromey's patch of yesterday.

import java.lang.ref.*;

public class Reftest {


    public void run(int id)
    {
        Object oa[] = new Object[1000];
        Reference ra[] = new Reference[oa.length];

        // Create a bunch of Objects and WeakReferences to them, but
        // also keep hard references to the objects so they don't get
        // GCed yet.
        for (int i = 0; i < oa.length; i++) {
            oa[i] = "Some String Id:"+id + " Number:" + i;
            ra[i] = new WeakReference(oa[i]);
        }
        //
        // Clear all the WeakReferences, then let GC deal with them.
        for (int i = 0; i < ra.length; i++) {
            ra[i].clear();
            ra[i] = null;
        }
        // GC can happen at anytime...  What a coincidence it is going
        // to happen now.
        System.gc();
        System.runFinalization();
        System.gc();
        System.runFinalization();
        System.gc();
        System.runFinalization();
        System.gc();
        System.runFinalization();
        

        // Allocate some more memory.  We will probably be using some
        // that used to be used by WeakReference objects.
        Object moreObjects1[] = new Object[100000];
        Object moreObjects2[] = new Object[moreObjects1.length];
        Object moreObjects3[] = new Object[moreObjects1.length];
        for(int i = 0; i < moreObjects1.length; i++) {
            moreObjects1[i] = "More Objects" + i;
            moreObjects2[i] = new java.util.ArrayList(10);
            moreObjects3[i] = new StringBuffer(20);
        }
        
        // Now the objects that were WeakReference referents are going to be GCable
        for(int i = 0; i < oa.length; i++) {
            oa[i] = null;
        }

        // GC thinks that it has to run again.
        System.gc();
        System.runFinalization();
        System.gc();
        System.runFinalization();
        System.gc();
        System.runFinalization();
        System.gc();
        System.runFinalization();

        // We cannot collect our top level objects because they are used here.
        System.out.println(oa.toString());
        System.out.println(ra.toString());
        System.out.println(moreObjects1.toString());
        System.out.println(moreObjects2.toString());
        System.out.println(moreObjects3.toString());
        

        System.out.println("iteration "+id+" finishing");
    }

    public Reftest() {
    }

    public static void main(String[] args) {
        System.setProperty("gnu.gcj.runtime.NameFinder.use_addr2line", "false");
        System.setProperty("gnu.gcj.runtime.NameFinder.sanitize", "false");
        System.setProperty("gnu.gcj.runtime.NameFinder.remove_unknown", "false");
        for (int i = 0; i < 20; i++) {
            Reftest reftest1 = new Reftest();
            reftest1.run(i + 1);
        }
    }
}

diff -u -b -B gcc-3.3.1/libjava/java/lang/ref/Reference.java mipsRebuild/gcc-3.3.1/libjava/java/lang/ref/Reference.java
--- gcc-3.3.1/libjava/java/lang/ref/Reference.java	Tue Nov 19 13:59:40 2002
+++ mipsRebuild/gcc-3.3.1/libjava/java/lang/ref/Reference.java	Thu Aug 21 08:56:03 2003
@@ -82,16 +82,25 @@
   gnu.gcj.RawData referent;
 
   /**
-   * This is like REFERENT but is not scanned by the GC.  We keep a
-   * copy around so that we can see when clear() has been called.
+   * This is like REFERENT but is not scanned by the GC.
    * GCJ LOCAL:
-   * This field doesn't exist in Classpath; we use it to detect
-   * clearing.
+   * This field doesn't exist in Classpath.  It is used internally in
+   * natReference.cc
    * END GCJ LOCAL
    */
   gnu.gcj.RawData copy;
 
   /**
+   * Set to true if {@link #clear()} is called.
+   * GCJ LOCAL:
+   * This field doesn't exist in Classpath.  It is used internally in
+   * natReference.cc, which enqueues the reference unless it is true
+   * (has been cleared).
+   * END GCJ LOCAL
+   */
+    boolean cleared = false;
+
+  /**
    * The queue this reference is registered on. This is null, if this
    * wasn't registered to any queue or reference was already enqueued.
    */
@@ -166,8 +175,12 @@
    */
   public void clear()
   {
+    // Must synchronize so changes are visible in finalizer thread.
+    synchronized(lock)
+      {
     referent = null;
-    copy = null;
+        cleared = true;
+      }
   }
 
   /**
diff -u -b -B gcc-3.3.1/libjava/java/lang/ref/natReference.cc mipsRebuild/gcc-3.3.1/libjava/java/lang/ref/natReference.cc
--- gcc-3.3.1/libjava/java/lang/ref/natReference.cc	Tue Nov 19 13:59:41 2002
+++ mipsRebuild/gcc-3.3.1/libjava/java/lang/ref/natReference.cc	Thu Aug 21 09:20:30 2003
@@ -1,6 +1,6 @@
 // natReference.cc - Native code for References
 
-/* Copyright (C) 2001, 2002  Free Software Foundation
+/* Copyright (C) 2001, 2002, 2003  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -67,6 +67,8 @@
 // Number of slots total in HASH.  Must be power of 2.
 static int hash_size = 0;
 
+#define DELETED_REFERENCE  ((jobject) -1)
+
 static object_list *
 find_slot (jobject key)
 {
@@ -89,7 +91,13 @@
 	    return &hash[deleted_index];
 	}
       else if (ptr->weight == DELETED)
+	{
+          // Use first deleted slot so future walks through the list
+          // are shorter.
+          if(deleted_index == -1)
 	deleted_index = index;
+	  JvAssert (ptr->reference == DELETED_REFERENCE);
+	}
       index = (index + step) & (hash_size - 1);
       JvAssert (index != start_index);
     }
@@ -132,6 +140,12 @@
   java::lang::ref::Reference *ref
     = reinterpret_cast<java::lang::ref::Reference *> (obj);
   object_list *head = find_slot (ref->copy);
+
+  // We might have found a new slot.  We can just ignore that here.
+  if (head->reference != ref->copy)
+      return;
+  
+
   object_list **link = &head->next;
   head = head->next;
 
@@ -168,7 +182,7 @@
   // Use `copy' here because the `referent' field has been cleared.
   jobject referent = the_reference->copy;
   object_list *item = find_slot (referent);
-  if (item->reference == NULL)
+  if (item->reference == NULL || item->reference == DELETED_REFERENCE)
     {
       // New item, so make an entry for the finalizer.
       item->reference = referent;
@@ -217,6 +231,7 @@
       // run, all the object's references have been processed, and the
       // object is unreachable.  There is, at long last, no way to
       // resurrect it.
+      list->reference = DELETED_REFERENCE;
       list->weight = DELETED;
       --hash_count;
       return;
@@ -249,7 +264,7 @@
 	    = reinterpret_cast<java::lang::ref::Reference *> (head->reference);
 	  // If the copy is already NULL then the user must have
 	  // called Reference.clear().
-	  if (ref->copy != NULL)
+	  if (!ref->cleared)
 	    ref->enqueue ();
 
 	  object_list *next = head->next;