Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fix: remove plugin directory handle without fail handles greater than the removed one. #2018

Closed
wants to merge 1 commit into from

3 participants

@ulion
Collaborator

currently, remove a handle will fail all other handles which greater than the removed one, this will happen when more than one plugindirectory script running within a overlapped time.
it's a obvious bug since the handle supposed not always be 0, there is possibility following example happens:

ui make a plugin dir listing request, which assigned handle 0:
plugin with handle 0 begin
json-rpc make another plugin dir listing request, which assigned handle 1:
plugin with handle 1 begin
plugin with handle 0 end, in the current code, the handle will be erased
plugin with handle 1 call function like setEndOfDirectory but handle 1 does not existed, the globalHandles vector size is 1 now the CPluginDirectory object with original handle 1 now on the handle 0 position, so the setEndOfDirectory call will failed, it's a mistake, this commit fix this problem.

btw, I'm writing addons, but have no machine to compile xbmc, so please test it first before merge to the main branch.

@jmarshallnz
Owner

I suggest grabbing the first NULL handle in getNewHandle() as well (should one exist). That way you'll cleanup the first handles as they are done. I'd also lock in dirFromHandle() as otherwise it is likely to be called later without the lock.

I'd also add a comment to the block that's cleaning out the handles at the end of the vector so it's clear what the code is doing.

Looks good other than that - thanks!

Collaborator

the first thought I also want to use the first NULL value handle, but I found always use greatest handle for the new one will let debug more easy, and there's nothing to lose in that condition but more debug friendly, so I choose this plan.
about the lock, you are right, I will try update this PR.

That's fine - update the PR with the fix (just squash the fix into this commit and force push to your repo) to the locking and I'll take another look before pulling it in.

Collaborator

I didn't known the squash stuff before this, now I known how to co-work on the git. thank you!

@ulion
Collaborator

thanks for reply, I had to close this pull req and using another one since this branch name is used by my previous request with a typo in the code, the issue still listing the old typo one since they share the same branch name.
I will reply your comment in the new issue

@ulion ulion closed this
@ulion ulion deleted the ulion:plugin_dir_handle_fix branch
@MartijnKaijser

next time you can just change your code and do a force push to your repo. saves opening a new PR

@ulion
Collaborator

thanks the suggesting, now I know how to do next time.

@ulion
Collaborator

and I still didn't figure out why pull 2019 not showed in the issue list. I already changed branch name but it's still only in the pull request list while not in the issue list.
any way, this issue and pull request was closed, please goto #2019 to continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
43 xbmc/filesystem/PluginDirectory.cpp
@@ -73,7 +73,23 @@ void CPluginDirectory::removeHandle(int handle)
{
CSingleLock lock(m_handleLock);
if (handle >= 0 && handle < (int)globalHandles.size())
- globalHandles.erase(globalHandles.begin() + handle);
+ {
+ globalHandles[handle] = NULL;
+ while (handle >= 0 && handle + 1 == (int)globalHandles.size() && NULL == globalHandles[handle])
+ {
+ globalHandles.erase(globalHandles.begin() + handle);
+ --handle;
+ }
+ }
+}
+
+// easy method to get dir object from handle.
+// lock only in the function maybe not strong enough, make sure lock before call it.
+CPluginDirectory *CPluginDirectory::dirFromHandle(int handle)
+{
+ if (handle >= 0 && handle < (int)globalHandles.size())
+ return globalHandles[handle];
+ return NULL;
}
bool CPluginDirectory::StartScript(const CStdString& strPath, bool retrievingDir)
@@ -161,13 +177,13 @@ bool CPluginDirectory::GetPluginResult(const CStdString& strPath, CFileItem &res
bool CPluginDirectory::AddItem(int handle, const CFileItem *item, int totalItems)
{
CSingleLock lock(m_handleLock);
- if (handle < 0 || handle >= (int)globalHandles.size())
+ CPluginDirectory *dir = dirFromHandle(handle);
+ if (NULL == dir)
{
CLog::Log(LOGERROR, " %s - called with an invalid handle.", __FUNCTION__);
return false;
}
- CPluginDirectory *dir = globalHandles[handle];
CFileItemPtr pItem(new CFileItem(*item));
dir->m_listItems->Add(pItem);
dir->m_totalItems = totalItems;
@@ -178,13 +194,13 @@ bool CPluginDirectory::AddItem(int handle, const CFileItem *item, int totalItems
bool CPluginDirectory::AddItems(int handle, const CFileItemList *items, int totalItems)
{
CSingleLock lock(m_handleLock);
- if (handle < 0 || handle >= (int)globalHandles.size())
+ CPluginDirectory *dir = dirFromHandle(handle);
+ if (NULL == dir)
{
CLog::Log(LOGERROR, " %s - called with an invalid handle.", __FUNCTION__);
return false;
}
- CPluginDirectory *dir = globalHandles[handle];
CFileItemList pItemList;
pItemList.Copy(*items);
dir->m_listItems->Append(pItemList);
@@ -196,12 +212,12 @@ bool CPluginDirectory::AddItems(int handle, const CFileItemList *items, int tota
void CPluginDirectory::EndOfDirectory(int handle, bool success, bool replaceListing, bool cacheToDisc)
{
CSingleLock lock(m_handleLock);
- if (handle < 0 || handle >= (int)globalHandles.size())
+ CPluginDirectory *dir = dirFromHandle(handle);
+ if (NULL == dir)
{
CLog::Log(LOGERROR, " %s - called with an invalid handle.", __FUNCTION__);
return;
}
- CPluginDirectory *dir = globalHandles[handle];
// set cache to disc
dir->m_listItems->SetCacheToDisc(cacheToDisc ? CFileItemList::CACHE_IF_SLOW : CFileItemList::CACHE_NEVER);
@@ -219,14 +235,13 @@ void CPluginDirectory::EndOfDirectory(int handle, bool success, bool replaceList
void CPluginDirectory::AddSortMethod(int handle, SORT_METHOD sortMethod, const CStdString &label2Mask)
{
CSingleLock lock(m_handleLock);
- if (handle < 0 || handle >= (int)globalHandles.size())
+ CPluginDirectory *dir = dirFromHandle(handle);
+ if (NULL == dir)
{
CLog::Log(LOGERROR, "%s - called with an invalid handle.", __FUNCTION__);
return;
}
- CPluginDirectory *dir = globalHandles[handle];
-
// TODO: Add all sort methods and fix which labels go on the right or left
switch(sortMethod)
{
@@ -556,12 +571,12 @@ bool CPluginDirectory::WaitOnScriptResult(const CStdString &scriptPath, int scri
void CPluginDirectory::SetResolvedUrl(int handle, bool success, const CFileItem *resultItem)
{
CSingleLock lock(m_handleLock);
- if (handle < 0 || handle >= (int)globalHandles.size())
+ CPluginDirectory *dir = dirFromHandle(handle);
+ if (NULL == dir)
{
CLog::Log(LOGERROR, " %s - called with an invalid handle.", __FUNCTION__);
return;
}
- CPluginDirectory* dir = globalHandles[handle];
dir->m_success = success;
*dir->m_fileResult = *resultItem;
@@ -613,13 +628,13 @@ void CPluginDirectory::SetContent(int handle, const CStdString &strContent)
void CPluginDirectory::SetProperty(int handle, const CStdString &strProperty, const CStdString &strValue)
{
CSingleLock lock(m_handleLock);
- if (handle < 0 || handle >= (int)globalHandles.size())
+ CPluginDirectory *dir = dirFromHandle(handle);
+ if (NULL == dir)
{
CLog::Log(LOGERROR, "%s called with an invalid handle.", __FUNCTION__);
return;
}
- CPluginDirectory *dir = globalHandles[handle];
if (strProperty == "fanart_image")
dir->m_listItems->SetArt("fanart", strValue);
else
View
1  xbmc/filesystem/PluginDirectory.h
@@ -71,6 +71,7 @@ class CPluginDirectory : public IDirectory
static std::vector<CPluginDirectory*> globalHandles;
static int getNewHandle(CPluginDirectory *cp);
static void removeHandle(int handle);
+ static CPluginDirectory *dirFromHandle(int handle);
static CCriticalSection m_handleLock;
CFileItemList* m_listItems;
Something went wrong with that request. Please try again.