Created attachment 14940 [details]
Abandon job if redirection is same as old dir.

Patch to stop kdirlister looping on a slotRedirection if the new and old paths
are identical.

I am wondering if it is the right place to fix (or perhaps not the only place to fix).

If a HTTP link gives an erroneous 302 error with a URL that is the same URL, would it not create an infinite loop with nearly all applications? (Or may be I have not understood this bug correctly?)

This is a specific issue here because the dirlister is assuming it is being given a directory name and doing a propfind on it so in fact we aren't actually requesting what the redirect is telling us.
Other apps like konqueror already detect redirection loops.

SVN commit 515665 by goutte:

Check if the redirection is for the same URL and give up if it is the case.

Note: I am commiting this now, as Debian bug #355111 seems to indicate 
a uncooperative behaviour toward the server (with a tendency to be a denial of
service). However I am not sure about the patch, as the URLs could differ
only by trailing slahes. I do not know if this would be valid redirection.
That is why I am not closing the bug either, to let an expert decide.
(If it is not valid, then KURL::equals should be used instead of
KURL::operator == )

CCBUG:123034


 M  +6 -0      kdirlister.cpp  


--- branches/KDE/3.5/kdelibs/kio/kio/kdirlister.cpp #515664:515665
@@ -1039,6 +1039,12 @@
   KURL oldUrl = job->url();  // here we really need the old url!
   KURL newUrl = url;
 
+  if ( oldUrl == newUrl )
+  {
+    kdDebug(7004) << k_funcinfo << "New redirection url same as old, giving up." << endl;
+    killJob(job);
+    return;
+  }
   // strip trailing slashes
   oldUrl.adjustPath(-1);
   newUrl.adjustPath(-1);

Bah, you committed 'my' fix :P... I too was waiting for feedback. :)

Anyway I believe the behaviour is correct because we are trying to spot an 
unexpected loop where we receive the same redirect we just issued.

The url's couldn't differ by trailing slashes in this instance the problem is 
being caused because we are appending a trailing slash when we use the url 
we've been given.

i.e. a redirect comes in to "/error.html" this is stored as the old url and 
the redirect issued, it then hits the server as "/error.html/" and a redirect 
is received. However when it then comes back into the server both old and new 
url's are "/error.html"

I think this can be closed then. I was just hoping for comment from a kio expert.

The question from Michael Brade (the kdirlister maintainer) is whether we couldn't detect
this in the slave instead; and if not, where we should really kill the job.

I didn't look into the problem enough to know if detecting from the slave is possible
(ivor, what do you think?) - but well, with N slaves possibly making this mistake I think
it would make sense for the kio framework to be robust against this.
On the other hand I agree with Michael that it would be good to understand the exact
reason for the no-op redirect since it's not a good thing to have in any case;
the KDirLister fix might be covering a much bigger underlying problem (see below).

About whether to kill the job: since redirection() is the last thing a slave emits
before finished()  (*), we shouldn't need to kill the job, do we?   ... 
Wait, are we really talking about listDir() over HTTP? I thought it didn't support it
(only about webdav). Ivor: is gwenview really creating a KDirLister for a HTTP URL?
Either gwenview or kdirlister should use KProtocolInfo::supportsListing(),
and kio_http should check in listDir() that a webdav url is given, not a http one...
(Hamish?)


(*) From get(), redirection() is just for information, the data follows.
From listDir(), redirection() means "I'm stopping here, please list that URL instead".

On Saturday 04 March 2006 16:24, Ivor Hewitt wrote:
> i.e. a redirect comes in to "/error.html" this is stored as the old url and 
> the redirect issued, it then hits the server as "/error.html/" and a redirect 
> is received. However when it then comes back into the server both old and new 
> url's are "/error.html"

This is what I have trouble following; who adds the '/' and who removes it, and
couldn't the slave notice oldurl==newurl?

Nicolas: there was no rush to commit the patch; let's get it straight first.

> Ivor: is gwenview really creating a KDirLister for a HTTP URL? 
>
It's the gvimagepart, that creates it. It is indeed creating a KDirLister. It takes the parent path of the URL it's just been given and then creates a KDirLister on it.
As far as I could tell the loop was within the dir lister and the only way I managed to stop the job was killing it inside the redirect call.

> Either gwenview or kdirlister should use KProtocolInfo::supportsListing(), 
>
Ah that sounds like the correct solution. However, I think retaining the loop detect would be worthwhile to stop broken apps... perhaps it should assert in debug mode?

> This is what I have trouble following; who adds the '/' and who removes it
> and couldn't the slave notice oldurl==newurl? 
>
I'm guessing in the depths of KIO::ListJob somewhere. Since it's assuming that it is being given a directory to work with.

aha, I think I've found a spot in kdirlister to do the "supportsListing" call... but off to work time.

listDir() in kio_http indeed had a bug allowing plain http:// urls to send PROPFIND requests - I have fixed this now.

Excellent, thanks Hamish. So can we revert the KDirLister patch then? Or rather, as I have said already, remove the killJob() call?

And then I would add the KProtocolInfo::supportsListing() call to the beginning of KDirListerCache::listDir(). Ivor, where would you have added the call? Just curious... :-)

I thought validURL looked like a promising candidate actually, just after the "//TODO verify this is a directory" comment.
But as commented... haven't got time to try it out yet.

Very good catch! Yes, that's equivalent functionality-wise, but validURL is a 
much nicer place indeed. 

I also thought about putting the validURL()-check in slotRedirection() to test 
the new url. But I'm not sure about what to do if the url is not valid: if we 
leave the job running it might keep emitting signals, possibly with that 
invalid url and that would crash KDirListerCache since it didn't update its 
data structures for that url. So in this case it might be ok to kill the job. 
David, thoughts?

On Monday 06 March 2006 21:54, Michael Brade wrote:
> I also thought about putting the validURL()-check in slotRedirection() to test 
> the new url. But I'm not sure about what to do if the url is not valid: if we 
> leave the job running it might keep emitting signals


No, since listDir() jobs are supposed to complete or error after emitting redirection,
they don't follow to the new url themselves (see kio_ftp for example).

So I think it's fine to ignore the redirection to an invalid url - although maybe
a messagebox might be in order? 

Created attachment 14986 [details]
New fix

Seems to do the trick.

Great, thanks, I will commit it with the following modifications:
  - validURL should be moved into KDirListerCache, it's not the business
    of a KDirLister to decide if the url is valid, KDirLister::validURL()
    should just call KDirListerCache::validURL().
  - leave the oldURL == newURL check in slotRedirection and just return,
    nothing to be done in this case, really
  - check the redirected URL for validity.

SVN commit 516606 by brade:

Gee, maybe I should actually commit that stuff, got carried away by my Diploma 
thesis and Zack's interview :P

This commit prevents bad urls from being passed to KDirLister, mainly urls that
do not support listing. It also prevents KDirListerCache from doing redundant
work.

Ivor, please check it out and see if it still works for you the way it should.
There's a message box coming up now as well, but I cannot put appropriate text
in it due to the string freeze.

BUG:123034


 M  +39 -24    kdirlister.cpp  
 M  +2 -1      kdirlister_p.h  


--- branches/KDE/3.5/kdelibs/kio/kio/kdirlister.cpp #516605:516606
@@ -33,6 +33,7 @@
 #include <kglobal.h>
 #include <kglobalsettings.h>
 #include <kstaticdeleter.h>
+#include <kprotocolinfo.h>
 
 #include "kdirlister_p.h"
 
@@ -84,7 +85,7 @@
 
 // setting _reload to true will emit the old files and
 // call updateDirectory
-void KDirListerCache::listDir( KDirLister* lister, const KURL& _u,
+bool KDirListerCache::listDir( KDirLister *lister, const KURL& _u,
                                bool _keep, bool _reload )
 {
   // like this we don't have to worry about trailing slashes any further
@@ -93,6 +94,9 @@
   _url.adjustPath(-1);
   QString urlStr = _url.url();
 
+  if ( !lister->validURL( _url ) )
+    return false;
+
 #ifdef DEBUG_CACHE
   printDebug();
 #endif
@@ -265,8 +269,36 @@
   // automatic updating of directories
   if ( lister->d->autoUpdate )
     itemU->incAutoUpdate();
+
+  return true;
 }
 
+bool KDirListerCache::validURL( const KDirLister *lister, const KURL& url ) const
+{
+  if ( !url.isValid() )
+  {
+    if ( lister->d->autoErrorHandling )
+    {
+      QString tmp = i18n("Malformed URL\n%1").arg( url.prettyURL() );
+      KMessageBox::error( lister->d->errorParent, tmp );
+    }
+    return false;
+  }
+
+  if ( !KProtocolInfo::supportsListing( url ) )
+  {
+    if ( lister->d->autoErrorHandling )
+    {
+      // TODO: this message should be changed during next string unfreeze!
+      QString tmp = i18n("Malformed URL\n%1").arg( url.prettyURL() );
+      KMessageBox::error( lister->d->errorParent, tmp );
+    }
+    return false;
+  }
+
+  return true;
+}
+
 void KDirListerCache::stop( KDirLister *lister )
 {
 #ifdef DEBUG_CACHE
@@ -1039,15 +1071,15 @@
   KURL oldUrl = job->url();  // here we really need the old url!
   KURL newUrl = url;
 
+  // strip trailing slashes
+  oldUrl.adjustPath(-1);
+  newUrl.adjustPath(-1);
+
   if ( oldUrl == newUrl )
   {
     kdDebug(7004) << k_funcinfo << "New redirection url same as old, giving up." << endl;
-    killJob(job);
     return;
   }
-  // strip trailing slashes
-  oldUrl.adjustPath(-1);
-  newUrl.adjustPath(-1);
 
   kdDebug(7004) << k_funcinfo << oldUrl.prettyURL() << " -> " << newUrl.prettyURL() << endl;
 
@@ -1819,9 +1851,6 @@
 
 bool KDirLister::openURL( const KURL& _url, bool _keep, bool _reload )
 {
-  if ( !validURL( _url ) )
-    return false;
-
   kdDebug(7003) << k_funcinfo << _url.prettyURL()
                 << " keep=" << _keep << " reload=" << _reload << endl;
 
@@ -1831,9 +1860,7 @@
 
   d->changes = NONE;
 
-  s_pCache->listDir( this, _url, _keep, _reload );
-
-  return true;
+  return s_pCache->listDir( this, _url, _keep, _reload );
 }
 
 void KDirLister::stop()
@@ -2182,19 +2209,7 @@
 
 bool KDirLister::validURL( const KURL& _url ) const
 {
-  if ( !_url.isValid() )
-  {
-    if ( d->autoErrorHandling )
-    {
-      QString tmp = i18n("Malformed URL\n%1").arg( _url.prettyURL() );
-      KMessageBox::error( d->errorParent, tmp );
-    }
-    return false;
-  }
-
-  // TODO: verify that this is really a directory?
-
-  return true;
+  return s_pCache->validURL( this, _url );
 }
 
 void KDirLister::handleError( KIO::Job *job )
--- branches/KDE/3.5/kdelibs/kio/kio/kdirlister_p.h #516605:516606
@@ -133,7 +133,8 @@
   KDirListerCache( int maxCount = 10 );
   ~KDirListerCache();
 
-  void listDir( KDirLister *lister, const KURL &_url, bool _keep, bool _reload );
+  bool listDir( KDirLister *lister, const KURL& _url, bool _keep, bool _reload );
+  bool validURL( const KDirLister *lister, const KURL& _url ) const;
 
   // stop all running jobs for lister
   void stop( KDirLister *lister );