Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix curl slist related double append and memory leak #2279

Merged
merged 2 commits into from Feb 25, 2013

Conversation

Projects
None yet
3 participants
Contributor

ulion commented Feb 22, 2013

there's 2 commit in this pr, related to the curl slist code.

  1. for the double append to m_curlAliasList problem.
  2. for the slist not released for Exists/Stat call of CurlFile.

@arnova arnova commented on an outdated diff Feb 22, 2013

xbmc/filesystem/CurlFile.cpp
@@ -467,7 +466,10 @@ void CCurlFile::SetCommonOptions(CReadState* state)
g_curlInterface.easy_setopt(h, CURLOPT_FAILONERROR, 1);
// enable support for icecast / shoutcast streams
- m_curlAliasList = g_curlInterface.slist_append(m_curlAliasList, "ICY 200 OK");
+ if ( NULL == m_curlAliasList )
+ // m_curlAliasList is used only by this one place, but SetCommonOptions can
+ // be called mutilple times, only append to list if it's empty.
@arnova

arnova Feb 22, 2013

Member

mutilple -> multiple ;-)

Contributor

ulion commented Feb 23, 2013

If no objection, this will be merged after 2 days.

Member

elupus commented Feb 23, 2013

You have an unrelated close change. When stat is used, open will not have
been called. Have you verified that it handles that?

I can live with the other change. It will not work if a inheriting class
set some additional aliases, but I suppose that is not used at the moment.

Contributor

ulion commented Feb 23, 2013

It's not unrelated, it just need to free the curl slists anyway, Exists/Stat only use the slist and not call Open, but slist need be free, so the Close() can be safely used here. Yes, I checked the Close() code, it's ok if no Open() get called before, mainly the m_state->Disconnect() is need to look into, and it's also clean.

Contributor

ulion commented Feb 24, 2013

@elupus I'm going to merge, ok with this?

Member

elupus commented Feb 25, 2013

Yup

@ulion ulion added a commit that referenced this pull request Feb 25, 2013

@ulion ulion Merge pull request #2279 from ulion/fix_curl_slist
Fix curl slist related double append and memory leak
ba38112

@ulion ulion merged commit ba38112 into xbmc:master Feb 25, 2013

@ulion ulion deleted the ulion:fix_curl_slist branch Apr 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment