harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Yu" <junjie0...@gmail.com>
Subject Re: [classlib][luni] java/util/HashMap.java bug waiting to happen?
Date Wed, 21 May 2008 02:19:51 GMT
Hi Oli,

I agree with you. (2) will be simple and can prevent potential risk.

2008/5/21 Oliver Deakin <oliver.deakin@googlemail.com>:

> Hi Mark,
>
> Agreed that this could cause problems in the future if it is called from
> another method expecting it to behave properly for entries that do not exist
> in the HashMap. Three options immediately spring to mind:
> 1) Leave it as is - we won't hit the non-existent entry problem at the
> moment due to the method caller always ensuring the entry exists.
> 2) Add a condition after the while loop to return without altering the
> HashMap if m.next is null.
> 3) Get rid of the removeEntry() method by inlining it into the calling
> method and fixing it up to handle non-existent entries.
>
> I think (2) is a simple fix, although (3) is also reasonable since there is
> only a single call to this method. I think (1) is, as you say, a problem
> waiting to happen.
> Personally I'd go with (2).
>
> Regards,
> Oliver
>
>
> Mark Hindess wrote:
>
>> While reviewing Aleksey's improvements in HARMONY-5791, I noticed the
>> following lines in HashMap.java (line 682):
>>
>>    final void removeEntry(Entry<K, V> entry) {
>>        int index = entry.origKeyHash & (elementData.length - 1);
>>        Entry<K, V> m = elementData[index];
>>        if (m == entry) {
>>            elementData[index] = entry.next;
>>        } else {
>> [1]         while (m.next != entry && m.next != null) {
>>                m = m.next;
>>            }
>> [2]         m.next = entry.next;
>>
>>        }
>>        modCount++;
>>        elementCount--;
>>    }
>>
>> The while loop at [1] has two conditions.  The first relates to finding
>> the entry and the second relates to getting to the end of the linked
>> list (i.e. not finding the entry).  Executing the line [2] only really
>> makes sense if the first condition was met; if the second was met then
>> it might corrupt the list.
>>
>> As it happens, the only caller of this method in HashMap.java does
>> ensure that the entry is present in the HashMap and thus in the linked
>> list but it still seems like a problem waiting to happen (or at the very
>> least a redundant condition on a while loop).
>>
>> It isn't broken so I'm not sure what (if anything) to "fix".
>>
>> Regards,
>> -Mark.
>>
>>
>>
>>
>>
>
> --
> Oliver Deakin
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> PO6 3AU
>
>


-- 
Best Regards,
Jim, Jun Jie Yu

China Software Development Lab, IBM

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message