Bug 304796 - ResourceData::resetAll() may get stuck in infinite cycle (resulting in crash)
Summary: ResourceData::resetAll() may get stuck in infinite cycle (resulting in crash)
Status: RESOLVED FIXED
Alias: None
Product: nepomuk
Classification: Miscellaneous
Component: libnepomukcore (show other bugs)
Version: git master
Platform: Ubuntu Linux
: NOR critical
Target Milestone: ---
Assignee: Vishesh Handa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-08 13:49 UTC by Tuukka Verho
Modified: 2012-12-01 20:23 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch to remove calls to Resource::fromResourceUri() in ResourceData::resetAll() (4.79 KB, patch)
2012-08-08 14:57 UTC, Tuukka Verho
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tuukka Verho 2012-08-08 13:49:44 UTC
In some cases the following line in Nepomuk2::ResourceData::resetAll()

m_rm->m_urlKickOff.remove( m_cache[NIE::url()].toUrl() );

may fail to clear the entry from m_urlKickOff because m_cache[NIE::url()].toUrl() evaluates to QUrl(""). This may lead to an infinite loop as warned by the comment:

// IMPORTANT:
// Remove from the kickOffList before removing from the resource watcher
// This is required cause otherwise the Resource::fromResourceUri creates a new
// resource which is correctly identified to the ResourceData (this), and it is
// then deleted, which calls resetAll and this cycle continues.


Reproducible: Always

Steps to Reproduce:
The following code always leads to an infinite loop and a crash:
{
  Nepomuk2::Resource res(QUrl("http://www.semanticdesktop.org/ontologies/2007/01/19/nie#title"));    
  res.property(QUrl("http://www.w3.org/TR/ol-ref/#range-def"));
}
Actual Results:  
The call to the destructor of the Resource object created in the second line leads to a call to ResourceData::resetAll() which ends up in an inifinite loop. A stack overflow results.

The stack trace consists of these lines repeated:

#107 Nepomuk2::ResourceData::resetAll() at resourcedata.cpp:173
#108 Nepomuk2::ResourceData::~ResourceData() at resourcedata.cpp:101
#109 Nepomuk2::Resource::~Resource() at resource.cpp:103
#110 Nepomuk2::Resource::~Resource() at resource.cpp:105
#111 QList<Nepomuk2::Resource>::node_destruct() at qlist.h:430
#112 QList<Nepomuk2::Resource>::free() at qlist.h:756
#113 Nepomuk2::ResourceData::resetAll() at resourcedata.cpp:173
#114 Nepomuk2::ResourceData::~ResourceData() at /resourcedata.cpp:101





The line which is responsible of creating another Resource instance for the URI and therefore creating an infinite loop is

if(m_rm->m_watcher->resources().count() == 1) {

If ResourceWatcher had a method count() that wouldn't create additional Resource objects (like resources() does) the risk of an infinite loop in corner cases might be avoided.
Comment 1 Vishesh Handa 2012-08-08 14:10:51 UTC
Thanks for the bug report.

First, however, you should never ever do this - 
 
> Nepomuk2::Resource res(QUrl("http://www.semanticdesktop.org/ontologies/2007/01/19/nie#title"));
> res.property(QUrl("http://www.w3.org/TR/ol-ref/#range-def"));

The resources should either be initialized by their resource uri or by the nie:url, which would be either something of the form '/home/user/asfadfa' or akonadi:?item=342. You're trying to initialize a resource with a property name.

That being said, we still shouldn't lead to an infinite loop and crash. We should gracefully output an error message, and inform you about evil doings.

I think ideally, we should not be using Resource inside the ResourceWatcher which is used in Resource. It leads to unnecessary complications like the one you have mentioned. Perhaps it would be better to have a special resource watcher just for the Resource class.

For now I'm adding the count function to the ResourceWatcher.
Comment 2 Vishesh Handa 2012-08-08 14:17:44 UTC
I've created a review request - https://git.reviewboard.kde.org/r/105927/

Could you please test it out?
Comment 3 Tuukka Verho 2012-08-08 14:57:49 UTC
Created attachment 73051 [details]
Patch to remove calls to Resource::fromResourceUri() in ResourceData::resetAll()
Comment 4 Tuukka Verho 2012-08-08 15:00:15 UTC
There was still another call to Resource::fromResourceUri() in ResourceData::resetAll(). The above patch removes that too by introducing an overloaded method ResourceWatcher::removeResource(const QUrl& uri).

Now the infinite loop is avoided. There's just a failed assert on exit:

ASSERT: "d->m_urlKickOff.isEmpty()" in file /home/tuukka/code/nepomuk-bug/nepomuk-core/libnepomukcore/resource/resourcemanager.cpp, line 219
Comment 5 Tuukka Verho 2012-08-08 15:01:34 UTC
> First, however, you should never ever do this - 
>  
> > Nepomuk2::Resource res(QUrl("http://www.semanticdesktop.org/ontologies/2007/01/19/nie#title"));
> > res.property(QUrl("http://www.w3.org/TR/ol-ref/#range-def"));

Then how to get the range of a property?
Comment 6 Vishesh Handa 2012-08-08 15:09:37 UTC
(In reply to comment #5)
> 
> > First, however, you should never ever do this - 
> >  
> > > Nepomuk2::Resource res(QUrl("http://www.semanticdesktop.org/ontologies/2007/01/19/nie#title"));
> > > res.property(QUrl("http://www.w3.org/TR/ol-ref/#range-def"));
> 
> Then how to get the range of a property?

#include <Nepomuk2/Types/Property>
#include <Nepomuk2/Types/Class>
#include <Nepomuk2/Vocabulary/NIE>

using namespace Nepomuk2::Vocabulary;

Nepomuk2::Types::Property nieTitle( NIE::title() );
kDebug() << nieTitle.range() << nieTitle.literalRangeType();

------------

http://api.kde.org/4.x-api/kdelibs-apidocs/nepomuk-core/html/classNepomuk2_1_1Types_1_1Property.html
Comment 7 Vishesh Handa 2012-08-08 15:13:05 UTC
(In reply to comment #3)
> Created attachment 73051 [details]
> Patch to remove calls to Resource::fromResourceUri() in
> ResourceData::resetAll()

Unfortunately, I cannot commit this, cause it changes the signature of a method. That breaks compatibility with previous versions.
Comment 8 Tuukka Verho 2012-08-08 15:17:46 UTC
(In reply to comment #7)
> (In reply to comment #3)
> > Created attachment 73051 [details]
> > Patch to remove calls to Resource::fromResourceUri() in
> > ResourceData::resetAll()
> 
> Unfortunately, I cannot commit this, cause it changes the signature of a
> method. That breaks compatibility with previous versions.

Ok. Well, I'm sure you'll come up with some long term solution.
Comment 9 Vishesh Handa 2012-08-08 15:48:37 UTC
Git commit 7bef7c53d3b9a971c203ed4391bf19ac79f381f5 by Vishesh Handa.
Committed on 08/08/2012 at 16:08.
Pushed by vhanda into branch 'KDE/4.9'.

ResourceWatcher: add *count() methods for resources, properties and types

These count methods are faster than returning the full list and then
counting the number of items in the list.

This also fixes an infinite loop where a temporary Resource is created
when querying for the number of resources being watched. The temporary
Resource is deleted, which results in ResourceData::resetAll being
callled which again called ResourceWatcher::resources(), and the cycle
countinues.
REVIEW: 105927

M  +16   -0    libnepomukcore/datamanagement/resourcewatcher.cpp
M  +27   -0    libnepomukcore/datamanagement/resourcewatcher.h
M  +1    -1    libnepomukcore/resource/resourcedata.cpp

http://commits.kde.org/nepomuk-core/7bef7c53d3b9a971c203ed4391bf19ac79f381f5
Comment 10 Tuukka Verho 2012-08-08 16:15:38 UTC
(In reply to comment #9)
> Git commit 7bef7c53d3b9a971c203ed4391bf19ac79f381f5 by Vishesh Handa.
> Committed on 08/08/2012 at 16:08.
> Pushed by vhanda into branch 'KDE/4.9'.

Maybe I didn't make it clear that the infinite loop persists as long as the second call to Resource::fromResourceUri is there:

QMetaObject::invokeMethod(m_rm->m_watcher, "removeResource", Qt::AutoConnection, Q_ARG(Nepomuk2::Resource, Resource::fromResourceUri(m_uri)));

So I wouldn't call this bug fixed yet.
Comment 11 Vishesh Handa 2012-08-08 16:17:47 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Git commit 7bef7c53d3b9a971c203ed4391bf19ac79f381f5 by Vishesh Handa.
> > Committed on 08/08/2012 at 16:08.
> > Pushed by vhanda into branch 'KDE/4.9'.
> 
> Maybe I didn't make it clear that the infinite loop persists as long as the
> second call to Resource::fromResourceUri is there:
> 
> QMetaObject::invokeMethod(m_rm->m_watcher, "removeResource",
> Qt::AutoConnection, Q_ARG(Nepomuk2::Resource,
> Resource::fromResourceUri(m_uri)));
> 
> So I wouldn't call this bug fixed yet.

I know. I should have reopened this :)
Comment 12 Tuukka Verho 2012-08-09 11:27:23 UTC
I managed to trigger this bug in a quite general situation:

{
Nepomuk2::Resource res(anyExistingResourceUri);
res.properties();
}

The reason is following: In ResourceData::load(), m_cache is cleared and rewritten if m_cacheDirty is true. However the entry m_cache[NIE::url()] is not rewritten and therefore the following line in resetAll() fails:

 m_rm->m_urlKickOff.remove( m_cache[NIE::url()].toUrl() );

Changing that line to

m_rm->m_urlKickOff.remove( m_uri );

removes the problem and avoids the infinite loop.
Comment 13 Tuukka Verho 2012-08-09 11:55:10 UTC
(In reply to comment #7)
> (In reply to comment #3)
> > Created attachment 73051 [details]
> > Patch to remove calls to Resource::fromResourceUri() in
> > ResourceData::resetAll()
> 
> Unfortunately, I cannot commit this, cause it changes the signature of a
> method. That breaks compatibility with previous versions.

Just a thought: I guess in 5.0 you have to make the methods in ResourceWatcher URI based instead of taking Resource objects as arguments, because ResourceWatcher is in datamanagement now. It doesn't make sense that a class in datamanagement depends on Resource.
Comment 14 Vishesh Handa 2012-11-30 23:15:07 UTC
I think this has gotten fixed over time. Could you please cross check. I've tried to read through the entire log, but I'm a little confused as to what the issue is.

Thanks.

I'm marking this as NEEDSINFO - WAITINGFORINFO. The wait is for you to confirm if it has been fixed. If it has, then please mark it as fixed.
Comment 15 Tuukka Verho 2012-12-01 20:12:11 UTC
Cannot reproduce the crash in 4.9.3. Hard to say what fixed it...