harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Deakin <oliver.dea...@googlemail.com>
Subject Re: [classlib] Fixing an unsafe reference in InputStream
Date Thu, 01 Oct 2009 17:11:05 GMT
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;
>              }
>
>
>   

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


Mime
View raw message