Calling finished() after error() in KIO slaves causes data corruption and crashes

[Vlad]

Hi,

The documentation for SlaveBase::error()

http://api.kde.org/4.0-api/kdelibs-apidocs/kio/html/classKIO_1_1SlaveBase.html#6938ad4b6647c3f0150ba77d06725efb

says that error() also finishes the job, so there's no need to call
finished(). That advice is stated as a guidance instead of a rule, and
consequently kio_smb and other KIO slaves have overlooked it. This is
bad because it can crash apps which use that IO slave and can lead to
data corruption, as in the following scenario:

1) JobA is started and uses the KIO slave
2) The KIO slave calls error()
3) A new job (jobB) is started and uses the same KIO slave
4) The KIO slave calls finished()
5) JobB terminates prematurely, and the error code is _not_ set

The data in jobB is incomplete, but the user app has no way of knowing
that because jobB->error() == 0. So the app thinks jobB's data is
valid and tries to use it, causing a crash or corruption.

I think the reason why this race condition wasn't discovered earlier
is that in order to trigger it, jobB needs to be started within a
short timespan, namely after error() but before the extraneous
finished() intended for jobA.

I think the best way to fix this would be to make any redundant calls
to finished() or error() truly not matter, either for the current job
or for future jobs that use the same KIO slave. In other words,
multiple error() and finished() calls for the same job shouldn't cause
any damage. Unfortunately, I don't know how to approach such a fix, so
I would appreciate any help you can offer.

Another solution would be to transform the documentation guidance into
a rule and try to enforce it. However, that is going to be hard
because finding existing instances where finished() is called after
error() is not trivial. Frankly, I can see how the instinct of KIO
slave writers will be to still call finished() after error(), just to
be safe and make sure it's finished.

Thanks,
Vlad


PS: Here's how finished() can end up being called after error() in
kio_smb_browse.cpp:

1) SMBSlave::stat() calls SMBSlave::browse_stat_path(), which calls
SMBSlave::reportError(), which calls error().
2) SMBSlave::stat() calls finished() if SMBSlave::browse_stat_path()
returns false, which it does when there's an error.

As you can see, this redundancy is not readily apparent, and there may
be other instances hiding in KIO slaves.


      ____________________________________________________________________________________
Looking for last minute shopping deals?  
Find them fast with Yahoo! Search.  http://tools.search.yahoo.com/newsearch/category.php?category=shopping