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;
>> }
>>
>>
>>
>
