harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Ellison <t.p.elli...@gmail.com>
Subject Re: [classlib] Fixing an unsafe reference in InputStream
Date Thu, 01 Oct 2009 20:38:12 GMT
Thanks guys, fixed at r820776.

Regards,
Tim

On 01/Oct/2009 18:11, Oliver Deakin wrote:
> Tim Ellison wrote:
>> Could somebody please take a look a the patch below and see if they
>> believe my explanation for why it fixes a findbugs problem in
>> InputStream?
>>
>> The problem:  We have a static, skipBuf, that we only use to read junk
>> data from the stream when skipping a number of bytes.  The static is
>> lazily initialized.  It may be null, or too small for the job.  The
>> problem is that another thread could also see null (or a small buffer)
>> and then set the buffer too small after the calling thread sets it to
>> /their/ required size.
>>
>> The solution:  If we take a local copy, localBuf, as a snapshot of
>> skipBuf before testing it, then even if the skipBuf value is slammed by
>> another thread we have our local copy.  We can also blindly overwrite
>> any value stored by other threads since they will be doing the same
>> thing.
>>
>> So the update of the static is still unsafe, but the usage of it is ok
>> -- and this is better than making it volatile.
>>
>> Agree?
>>   
> 
> +1 from me.
> 
> Regards,
> Oliver
> 
>> Tim
>>
>>
>> Index: InputStream.java
>> ===================================================================
>> --- InputStream.java    (revision 820251)
>> +++ InputStream.java    (working copy)
>> @@ -211,11 +211,16 @@
>>          }
>>          long skipped = 0;
>>          int toRead = n < 4096 ? (int) n : 4096;
>> -        if (skipBuf == null || skipBuf.length < toRead) {
>> -            skipBuf = new byte[toRead];
>> +        // We are unsynchronized, so take a local copy of the skipBuf
>> at some
>> +        // point in time.
>> +        byte[] localBuf = skipBuf;
>> +        if (localBuf == null || localBuf.length < toRead) {
>> +            // May be lazily written back to the static. No matter if it
>> +            // overwrites somebody else's store.
>> +            skipBuf = localBuf = new byte[toRead];
>>          }
>>          while (skipped < n) {
>> -            int read = read(skipBuf, 0, toRead);
>> +            int read = read(localBuf, 0, toRead);
>>              if (read == -1) {
>>                  return skipped;
>>              }
>>
>>
>>   
> 

Mime
View raw message