[RFC] Fix IndexOutOfBoundException because of corrupted entry in recently_used file
Thomas Meyer
thomas at m3y3r.de
Thu Apr 5 06:30:51 PDT 2012
Am Donnerstag, den 05.04.2012, 14:59 +0200 schrieb Jiri Vanek:
> On 04/04/2012 04:57 PM, Thomas Meyer wrote:
> > Am Dienstag, den 03.04.2012, 16:50 +0200 schrieb Jiri Vanek:
> >> > Thanx for test! Logic itself is good, but there are two mistakes which causes the test to be not-working.
> >> > See notes inline for them.
> > Hi,
> >
> > I hacked a bit on this today. please have a look at the attached patch.
> >
> > I moved all the format checks in the CacheLRUWrapper.load() method. that
> > is the place were I think they actually belong.
> >
> > I guess these changes will break the CacheReproducerTest as no more
> > LRUExceptions are thrown.
> I would like to notice outside world that cache was corrupted. (LruCacheException)
> ex.printStacktrace can be as good as anything else (and is backward compatible with existing approach.
>
> However to print stacktrace just in debug mode will be probably better idea.
> Plus information text I have dared to add.
fine for me.
> >
> > what do you think about this approach? does this make sense for you?
>
> Yap! I till it excellent. It is also handling multiple launches upon broken cache better then my
> original fix did.
> Although there was an bug in removing item from cache with running iteration (which was causing
> ConcurrentModificationException)
oops. good catch. thanks.
>
> I have dared to fix this and LruCacheException printing and to include working tests to your
> original patch. This "improved" one is attached.
looks good to me.
>
> However still several questions to your original one in-line:
see my answers below.
> >
> >
> > check-cache-data.patch
> >
> >
> > diff -r 093896b370d3 Makefile.am
> > --- a/Makefile.am Wed Mar 28 12:08:10 2012 +0200
> > +++ b/Makefile.am Wed Apr 04 16:48:09 2012 +0200
> > @@ -565,7 +565,7 @@
> > cd $(JNLP_TESTS_ENGINE_DIR) ; \
> > class_names=`cat $(REPRODUCERS_CLASS_NAMES)` ; \
> > CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):. \
> > - $(BOOT_DIR)/bin/java -Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/javaws \
> > + $(BOOT_DIR)/bin/java -Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/$(javaws) \
> > -Xbootclasspath:$(RUNTIME) CommandLine $$class_names \
> > > stdout.log 2> stderr.log ; \
> > cat stdout.log ; \
> > diff -r 093896b370d3 netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> > --- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Mar 28 12:08:10 2012 +0200
> > +++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Apr 04 16:48:09 2012 +0200
> > @@ -107,6 +107,49 @@
> > */
> > public synchronized void load() {
> > cacheOrder.load();
> > +
> > + /*
> > + * clean up possibly corrupted entries
> > + */
> It is interesting to see how many times is checkData called during single jar run.. I counted
> 10executions.... But fixing this is definitely work for different patch.
yeah, I also noticed this. maybe this can be reduced a bit. see below
comment for getCacheFile(). but as the recently_used file could be
corrupted by another jvm process (etc.), we need to check this on every
load.
> > + if(checkData())
> > + store();
> > +
> > + }
> > +
> > + /**
> > + * check content of cacheOrder and remove invalid/corrupt entries
> > + */
> > + private boolean checkData () {
> > + boolean modified = false;
> > +
> > + for (Entry<Object, Object> currentEntry : cacheOrder.entrySet()) {
> > + final String key = (String) currentEntry.getKey();
> > + final String path = (String) currentEntry.getValue();
> > +
> > + // 1. check key format: "milliseconds,number"
> > + try {
> > + String sa[] = key.split(",");
> > + Long l1 = Long.parseLong(sa[0]);
> > + Long l2 = Long.parseLong(sa[1]);
> > + } catch (Exception ex) {
> > + cacheOrder.remove(key);
> > + modified = true;
> > + continue;
> > + }
> > +
> > + // 2. check path format - does the path look correct?
> > + if (path != null) {
> > + if (path.indexOf(cacheDir)< 0) {
> > + cacheOrder.remove(key);
> > + modified = true;
> > + }
> > + } else {
> > + cacheOrder.remove(key);
> > + modified = true;
> > + }
> > + }
> > +
> > + return modified;
> > }
>
> Really elegant solution this check :)
> >
> > /**
> > @@ -174,15 +217,11 @@
> > Collections.sort(entries, new Comparator<Entry<String, String>>() {
> > @Override
> > public int compare(Entry<String, String> e1, Entry<String, String> e2) {
> > - try {
> > - Long t1 = Long.parseLong(e1.getKey().split(",")[0]);
> > - Long t2 = Long.parseLong(e2.getKey().split(",")[0]);
> > + Long t1 = Long.parseLong(e1.getKey().split(",")[0]);
> > + Long t2 = Long.parseLong(e2.getKey().split(",")[0]);
> >
> > - int c = t1.compareTo(t2);
> > - return c< 0 ? 1 : (c> 0 ? -1 : 0);
> > - } catch (Exception e) {
> > - throw new LruCacheException(R("Corrupt LRU file entries"));
> > - }
> > + int c = t1.compareTo(t2);
> > + return c< 0 ? 1 : (c> 0 ? -1 : 0);
> > }
> > });
> > return entries;
> > diff -r 093896b370d3 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 Apr 04 16:48:09 2012 +0200
> > @@ -308,7 +308,6 @@
> > if (cacheFile == null) { // We did not find a copy of it.
> > cacheFile = makeNewCacheFile(source, version);
> > }
> > - lruHandler.store();
> This scares me - why it have begin to be removed?
in getCacheFile() method this happens:
1.) call load() -> this must happen, because of concurrent update by
other jvm processes.
2.) checks if entry exists
2a.) when yes -> return the cacheFile entry
2b.) when no -> call makeNewCacheFile() -> will call load() and
store().
3.) call store() again -> so this one can be omitted as the store will
happen in makeNewCacheFile(), in already existing entry case, no store
is needed.
4.) return cacheFile entry
so here a superfluous store() is removed, as the store() will happen in
makeNewCacheFile() anyway. we may can get rid of the superfluous load()
in makeNewCacheFile() in this case, too?
> > lruHandler.unlock();
> > }
> > return cacheFile;
> > @@ -324,47 +323,19 @@
> > synchronized (lruHandler) {
> > File cacheFile = null;
> > int tries = 0;
> > - List<Entry<String, String>> entries = null;
> > - do {
> > - try {
> > - tries++;
> > - entries = lruHandler.getLRUSortedEntries();
> > - } catch (LruCacheException ex) {
> > - if (tries == 1) {
> > - ex.printStackTrace();
> > - System.out.println(R("CFakeCache"));
> > - lruHandler.clearLRUSortedEntries();
> > - lruHandler.store();
> > - System.out.println(R("CFakedCache"));
> > - } else if (tries == 2) {
> > - ex.printStackTrace();
> > - System.out.println(R("CStillCorupted"));
> > - boolean clearingresult = CacheUtil.clearCache();
> > - if (!clearingresult) {
> > - throw new InternalError(R("CCleaningUnsuccessful"));
> > - }
> > - System.out.println(R("CClearedReloading"));
> > - lruHandler.clearLRUSortedEntries();
> > - lruHandler.store();
> > - System.out.println(R("CReloadRestarting"));
>
> Properties texts should be removed to. I have dared to do this. and to reuse little bit modified two
> of them.
correct, I missed that. thanks
>
> > + List<Entry<String, String>> entries = lruHandler.getLRUSortedEntries();
> >
> > - } else {
> > - throw new InternalError(R("CStillBroken"));
> > - }
> > -
> > - }
> > - } 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();
> >
> > - 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.
> > - }
> > + assert(path!=null);
> I'm not friend of assert(...)....
I don't care about asserts(). They don't hurt as they are not active
anyway.
> I think this is even not necessary - the self-explaining NullPointerException will be thrown little
> bit later. However I do not insists and do as you wish.
Feel free to remove the asserts. I automatically write them in the code,
because they don't really hurt.
> > +
> > + if (pathToURLPath(path).equals(urlPath.getPath())) { // Match found.
> > + cacheFile = new File(path);
> > + lruHandler.updateEntry(key);
> > + break; // Stop searching since we got newest one already.
> > }
> > }
> > return cacheFile;
> > @@ -561,10 +532,13 @@
> > * This will remove all old cache items.
> > */
> > public static void cleanCache() {
> > +
> > if (okToClearCache()) {
> > // First we want to figure out which stuff we need to delete.
> > HashSet<String> keep = new HashSet<String>();
> > HashSet<String> remove = new HashSet<String>();
> > +
> > + // FIXME: lock on recently_used file needed?
> > lruHandler.load();
>
> hmmm .. I do not know:(
I guess it is not okay, as another javaws process could update the
recently_used file concurrently, maybe nobody did hit this situation. I
think the cleanChache() should be protected by a FileLock...
> >
> > long maxSize = -1; // Default
> > @@ -579,57 +553,57 @@
> > for (Entry<String, String> e : lruHandler.getLRUSortedEntries()) {
> > // Check if the item is contained in cacheOrder.
> > final String key = e.getKey();
> > - final String value = e.getValue();
> > + final String path = e.getValue();
> >
> > - if (value != null) {
> > - File file = new File(value);
> > - PropertiesFile pf = new PropertiesFile(new File(value + ".info"));
> > - boolean delete = Boolean.parseBoolean(pf.getProperty("delete"));
> > + assert(path!=null);
> same assert as above...?...
yes. path must be not null here.
> >
> > - /*
> > - * This will get me the root directory specific to this cache item.
> > - * Example:
> > - * cacheDir = /home/user1/.icedtea/cache
> > - * file.getPath() = /home/user1/.icedtea/cache/0/http/www.example.com/subdir/a.jar
> > - * rStr first becomes: /0/http/www.example.com/subdir/a.jar
> > - * then rstr becomes: /home/user1/.icedtea/cache/0
> > - */
> > - String rStr = file.getPath().substring(cacheDir.length());
> > - rStr = cacheDir + rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
> > - long len = file.length();
> > + File file = new File(path);
> > + PropertiesFile pf = new PropertiesFile(new File(path + ".info"));
> > + boolean delete = Boolean.parseBoolean(pf.getProperty("delete"));
> >
> > - if (keep.contains(file.getPath().substring(rStr.length()))) {
> > - lruHandler.removeEntry(key);
> > - continue;
> > - }
> > -
> > - /*
> > - * we remove entries from our lru if any of the following condition is met.
> > - * Conditions:
> > - * - delete: file has been marked for deletion.
> > - * - !file.isFile(): if someone tampered with the directory, file doesn't exist.
> > - * - maxSize>= 0&& curSize + len> maxSize: If a limit was set and the new size
> > - * on disk would exceed the maximum size.
> > - */
> > - if (delete || !file.isFile() || (maxSize>= 0&& curSize + len> maxSize)) {
> > - lruHandler.removeEntry(key);
> > - remove.add(rStr);
> > - } else {
> > - curSize += len;
> > - keep.add(file.getPath().substring(rStr.length()));
> > + /*
> > + * This will get me the root directory specific to this cache item.
> > + * Example:
> > + * cacheDir = /home/user1/.icedtea/cache
> > + * file.getPath() = /home/user1/.icedtea/cache/0/http/www.example.com/subdir/a.jar
> > + * rStr first becomes: /0/http/www.example.com/subdir/a.jar
> > + * then rstr becomes: /home/user1/.icedtea/cache/0
> > + */
> > + String rStr = file.getPath().substring(cacheDir.length());
> > + rStr = cacheDir + rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
> > + long len = file.length();
> >
> > - for (File f : file.getParentFile().listFiles()) {
> > - if (!(f.equals(file) || f.equals(pf.getStoreFile()))){
> > - try {
> > - FileUtils.recursiveDelete(f, f);
> > - } catch (IOException e1) {
> > - e1.printStackTrace();
> > - }
> > - }
> > + if (keep.contains(file.getPath().substring(rStr.length()))) {
> > + lruHandler.removeEntry(key);
> > + continue;
> > + }
> > +
> > + /*
> > + * we remove entries from our lru if any of the following condition is met.
> > + * Conditions:
> > + * - delete: file has been marked for deletion.
> > + * - !file.isFile(): if someone tampered with the directory, file doesn't exist.
> > + * - maxSize>= 0&& curSize + len> maxSize: If a limit was set and the new size
> > + * on disk would exceed the maximum size.
> > + */
> > + if (delete || !file.isFile() || (maxSize>= 0&& curSize + len> maxSize)) {
> > + lruHandler.removeEntry(key);
> > + remove.add(rStr);
> > + continue;
> > + }
> > +
> > + curSize += len;
> > + keep.add(file.getPath().substring(rStr.length()));
> > +
> > + for (File f : file.getParentFile().listFiles()) {
> > + if (!(f.equals(file) || f.equals(pf.getStoreFile()))){
> > + try {
> > + FileUtils.recursiveDelete(f, f);
> > + } catch (IOException e1) {
> > + e1.printStackTrace();
> > }
> > }
> > - } else {
> > - lruHandler.removeEntry(key);
> > +
> > }
>
> I must admit that I did not got fully the meaning of this change. But it is working pretty fine and
> looks ok :)
the idea is to remove the check for "if(path != null)", as we did
already make sure in load()->checkData() that the path is not null.
that's all.
> > }
> > lruHandler.store();
> > diff -r 093896b370d3 netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> > --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Mar 28 12:08:10 2012 +0200
> > +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Apr 04 16:48:09 2012 +0200
> > @@ -713,6 +713,7 @@
> > Runtime.getRuntime().addShutdownHook(new Thread() {
> > public void run() {
> > markNetxStopped();
> > + // no lock on recently_used file needed?
> I'm not sure again as above :(
> > CacheUtil.cleanCache();
> > }
> > });
> >
>
> I hope my small changes will not insult you, but while I was playing with your changeset they looks
> to me like essential ones so I put them in instead of describing.
I couldn't see any insults in this mail :-) Just good improvements to me
patch.
>
> If you will agree with printing of lrucacheexception stacktrace then this is nearly done.
Im' fine with the printing.
> If you will insists on no stacktreace printing or (what I prefer) on printing it out just in debug
> mode, then the tests will need little fixing. (for case with no stacktraces it will need to change
> testing conditions, and for stacktace printed in debugmode it will need to run javaws in verbose mode)
>
> Best regards and thanx for patch.
my pleasure.
with kind regards
thomas
More information about the distro-pkg-dev
mailing list