Bug 4129 - Correction to earlier AttributesImpl bug-fix
Summary: Correction to earlier AttributesImpl bug-fix
Status: NEW
Alias: None
Product: XmlCommons - Now in JIRA
Classification: Unclassified
Component: SAX (show other bugs)
Version: 1.x
Hardware: Other other
: P3 normal (vote)
Target Milestone: ---
Assignee: Commons Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2001-10-12 13:17 UTC by Glenn Marcy
Modified: 2004-11-16 19:05 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Glenn Marcy 2001-10-12 13:17:35 UTC
AttributesImpl.java includes a "fix" that changed the original:

    public void removeAttribute (int index)
    {
        if (index >= 0 && index < length) {
              if (index < length - 1) {
                System.arraycopy(data, (index+1)*5, data, index*5,
                                 (length-index)*5);
            }
            length--;
        } else {
            badIndex(index);
        }
    }

to:

    public void removeAttribute (int index)
    {
        if (index >= 0 && index < length) {
            data[index*5] = null;
            data[index*5+1] = null;
            data[index*5+2] = null;
            data[index*5+3] = null;
            data[index*5+4] = null;
            if (index < length - 1) {
                System.arraycopy(data, (index+1)*5, data, index*5,
                                 (length-index-1)*5);
            }
            length--;
        } else {
            badIndex(index);
        }
    }

The original code was clearing the entry at the new end of the list by copying 
the presumably empty entry "one past the end" over the last entry.  The change 
correctly changed the length of the copy to no longer reference past the end of 
the data array, which would sometimes raise an ArrayIndexOutOfBoundsException.

However, the code to clear the element being removed just before it is copied 
over can only be useful in the single case where the copy doesn't happen, i.e. 
the attr removed is the last one in the list.  The correct code would always 
clear the entry that actually needs to be cleared, which is the one that was 
previously in the last position before one was removed.  The following code does 
so:

    public void removeAttribute (int index)
    {
        if (index >= 0 && index < length) {
            if (index < length - 1) {
                System.arraycopy(data, (index+1)*5, data, index*5,
                                 (length-index-1)*5);
            }
            length--;
            data[length*5] = null;
            data[length*5+1] = null;
            data[length*5+2] = null;
            data[length*5+3] = null;
            data[length*5+4] = null;
        } else {
            badIndex(index);
        }
    }
Comment 1 Shane Curcuru 2001-10-15 16:27:10 UTC
Do we have a test program that shows the effects of this bug on another Apache 
product yet?
Comment 2 Glenn Marcy 2001-10-16 05:09:53 UTC
I am not sure how to create one exactly.  The reason for the initial report is 
not know to me, except that from inspection I can see that the original code did 
not clear the "slots" following the new last entry after a call to remove an 
attribute.  This would simply leave references to String objects that would not 
be collectable during gc(), but would otherwise seem to be benign (they are 
immutable after all...)  My changes were to simply take the "spirit" of the 
original fix and change the code to clear the String references so that the 
entries in the array were always null beyond the last in-use entry.  Perhaps we 
should ask garyp or dims if they have more information to offer.