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 a minor possible memory leak. #1185

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

zewt commented Jul 19, 2012

If file.Read() throws, then temp wasn't freed. Fix by reading directly
into checksum, to avoid direct memory management in the first place.

@zewt zewt Fix a minor possible memory leak.
If file.Read() throws, then temp wasn't freed.  Fix by reading directly
into checksum, to avoid direct memory management in the first place.
254e055
Member

jmarshallnz commented Jul 19, 2012

How does CFile::Read throw in this case?

The try/catch is there to stop if the user sets an MD5 file that turns out to be 8 GB big...

That doesn't negate the fix, but given there's issues assuming that GetLength() will return non-zero for some URLs anyway, I suspect it might be fixed in other ways.

@elupus, @cptspiff - googlecode URLs, eg:

http://bossanova808-xbmc-addons.googlecode.com/git/repository-downloads/addons.xml.md5

return Transfer-Encoding=chunked, which means CCurlFile::GetLength() returns 0 here, and thus the checksum is never read or stored.

Member

jmarshallnz commented Jul 19, 2012

Trac ticket for the latter here: http://trac.xbmc.org/ticket/13190

Contributor

zewt commented Jul 19, 2012

FWIW, it would be useful for code that broadly ignores exceptions to mention why, since without an exception type in there it's hard to tell in isolation (or, narrow the catch to bad_alloc).

It'd definitely be good to handle T-E: chunked (you'll get that from a lot of servers that use deflate, IIRC), though short of reading the whole stream in when GetLength is called, that's tricky to do generically with an API that pretends to be a plain file...

Contributor

zewt commented Jul 19, 2012

Note that there's another bug that this at least reduces: the stream might close before returning the advertised number of bytes. Previously, this would result in uninitialized heap bytes being dumped into the string; now it'll always result in zeroes, since strings initialize to null.

This still isn't really correct; rather than silently returning a partial file, it should either resume/restart the file or return "" as if no data was received at all. This seems like a reasonable short-term improvement, though; returning random stale data on the heap is scary, even if it doesn't cause obvious problems.

Contributor

bossanova808 commented Jul 22, 2012

Looks like spiff has worked on the greater issue here, right?

#1195

@zewt zewt Fix reads from Transfer-Encoding: chunked servers.
There's no Content-Length in this mode, so GetLength() returns 0.
d05fa35
@ghost

ghost commented Jul 22, 2012

please squash.

Contributor

zewt commented Jul 22, 2012

Done; #1200.

@zewt zewt closed this Jul 22, 2012

@ghost

ghost commented Jul 22, 2012

pushed in squashed form. thx

@tru tru added a commit to plexinc/plex-home-theater-public that referenced this pull request May 19, 2014

@tru tru Fix multiple-media selection.
Also add comprehensive tests for it.

Fixes #1185
842a120

@tru tru added a commit to RasPlex/plex-home-theatre that referenced this pull request Aug 21, 2014

@tru @LongChair tru + LongChair Fix multiple-media selection.
Also add comprehensive tests for it.

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