[RFC] Fix IndexOutOfBoundException because of corrupted entry in recently_used file

[Thomas Meyer]

Am Montag, den 02.04.2012, 15:14 +0200 schrieb Jiri Vanek:
> On 03/28/2012 02:41 PM, Thomas Meyer wrote:
> > # HG changeset patch
> > # User Thomas Meyer<thomas at m3y3r.de>
> > # Date 1332937976 -7200
> > # Node ID f6540088f06f2d9962e1c5b7858c4212f045759e
> > # Parent  093896b370d3ed3f1fc3527084133b8e388bf0ae
> > Fix IndexOutOfBoundException because of corrupt "recently_used" index file of cached files. Better solution would be to do this validation in CacheLRUWrapper.load()
> >
> > diff -r 093896b370d3 -r f6540088f06f netx/net/sourceforge/jnlp/cache/CacheUtil.java
> > --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java	Wed Mar 28 12:08:10 2012 +0200
> > +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java	Wed Mar 28 14:32:56 2012 +0200
> > @@ -354,17 +354,22 @@
> >
> >                   }
> >               } while (entries == null);
> > +
> >               // Start searching from the most recent to least recent.
> >               for (Entry<String, String>  e : entries) {
> >                   final String key = e.getKey();
> > -                final String path = e.getValue();
> > +                String path = e.getValue();
> >
> >                   if (path != null) {
> > -                    if (pathToURLPath(path).equals(urlPath.getPath())) { // Match found.
> > -                        cacheFile = new File(path);
> > -                        lruHandler.updateEntry(key);
> > -                        break; // Stop searching since we got newest one already.
> > -                    }
> > +                    path=pathToURLPath(path);
> > +                    if(path == null)
> > +                        lruHandler.removeEntry(key);
> > +                    else
> > +                        if (path.equals(urlPath.getPath())) { // Match found.
> > +                            cacheFile = new File(path);
> > +                            lruHandler.updateEntry(key);
> > +                            break; // Stop searching since we got newest one already.
> > +                        }
> >                   }
> >               }
> >               return cacheFile;
> > @@ -377,6 +382,13 @@
> >       private static String pathToURLPath(String path) {
> >           int len = cacheDir.length();
> >           int index = path.indexOf(File.separatorChar, len + 1);
> > +
> > +        /*
> > +         * somehow the lru entries got corrupt. ignore this entry and
> > +         * don't fail with an IndexOutOfBoundsException
> > +         */
> > +        if(index<  0)
> > +            return null;
> >           return path.substring(index);
> >       }
> >
> >
> >
> 
> Hi there! What are the prerequisites leading to do this  state?

Hi,

I'm not sure what did lead to this state. I found this nasty
constellation last friday. Maybe my browser crashed or something.

> I have fixed lru cache once already (for throwing NonIntegerValue exception) but I have never seen 
> this error even when I was trying to corrupt cache, and I agree with you that this should be done in 
> CacheLRUWrapper. Can you please provide (plaintext is enough) some reproducer?

While debugging this problem I found an entry in the recently_used file
that looked like this:

"1333048426514,0=/h"

> If you will be willing to add this behaviour to 
> icedtea-web/tests/jnlp_tests/signed/CacheReproducer/testcases/CacheReproducerTest.java it will be 
> really great (You should do this according to commit policy O:) "IcedTea-Web code changes/new 
> feature should be accompanied with appropriate tests (JUnit class and/or reproducer). If no tests 
> are added/modified, changes should be accompanied with an explanation as to why. " O:)

I will try this and post a patch the next days.

> 
>  From cache record eg.:
> 1332779007444,35=/home/user/.icedtea/cache/35/http/localhost/SetContextClassLoader.jnlp
> 
> I see that this is removing "/home/user/.icedtea/cache/" correct?

Yes, correct.

> 
> So it can fail when:
>   - recentlyused file have moved
>   - javaws was killed during writeing

I guess the saving of the recently_used file somehow did not finish.

> 
> In both cases I would prefer cleaning of cache instead of neverending removing of entry.

okay.

> 
> So after "lruHandler.removeEntry(key);" should be save() at least to prevent this to happen again 
> and again. But this brings other issues (like touching cache of other running javaws)

yes, indeed.

> 
> However, As best for fix this issue I'm considering change in CacheLRUWrapper's 
> getLRUSortedEntries() to put to value already prepared path (or object with both prepared/full paths).

I think that's the more correct thing to do.

> Then IndexOutOfBounds will be wrapped by LruCache exception and cache will be properly cleared.
> However the cleanCache() method in CacheUtil will need small fixing after this too.

Yes, probably.

> What do you think?

Sounds good to me.

btw. can the tests in the "test/jnlp_tests" directory be put in a
different directory and/or package, so I can add the "jnlp_tests"
directory to the eclipse build directory and start all tests in this
directory from the IDE? this works great for "test/netx/unit".

> 
> 
> Thank you in advance, and Tyvm for getting the issue!

my pleasure.

thomas