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 More Coverity Static Analysis Issues in XBMC #1355

Closed
wants to merge 72 commits into
from

Conversation

Projects
None yet
6 participants
Member

kylhill commented Sep 2, 2012

I've managed to get XBMC added to the list of Open Source projects that is scanned for free by the Coverity Static Analysis tool. See http://scan.coverity.com/ for more details about this program.

For other developers that are interested in viewing the static analysis output, please contact me for login information.

This pull request aggregates more of my commits to fix these static analysis issues.

@ghost ghost assigned elupus and Montellese Sep 2, 2012

Owner

Montellese commented Sep 4, 2012

Just looked through some of the commits and added some comments. The second commit (merge-commit of upstream/master) has to go otherwise this will never be merged. Generally this looks like it contains a lot of valid fixes for potential problems.

Looks like there are several fixes involving CRegExp::GetReplaceString because it mallocs a char * in the method which needs to be freed outside of the method. Would it make sense to instead change GetReplaceString to return an std::string and do the call to free() inside GetReplaceString? Sounds more fool-proof to me.

Member

kylhill commented Sep 4, 2012

@Montellese, we could replace GetReplaceString and I agree that it would be more fool-proof, but I'd rather do that in a separate pull request.

Owner

Memphiz commented Sep 4, 2012

I have looked through it and it looks fine. I'm unsure with:

58d706d292d7457cded7d77f99ecb441f16ab7cc

and

e520fafcac37f5ccfa03e928ec8cd51458ab8d51

Beside that - great work on finding these.

Kyle Hill added some commits Sep 1, 2012

Kyle Hill Fix memory leak in ShoutcastFile.cpp a8de832
Kyle Hill Fix uninitialized members in ScraperParser.cpp 9a49cc9
Kyle Hill Fix uninitialized member in DVDPlayerAudio.cpp 1797ff3
Kyle Hill Fix use of uninitialized member in CrystalHD.cpp e309e20
Kyle Hill Fix read past the end of m_features arrray in VDPAU
It looks like there should actully be 14 elements in this array, rather
than 13.
1423a65
Kyle Hill Fix possible integer overflow in Edl.cpp 0567eb1
Kyle Hill No need to compare an array against NULL in DVDFileInfo.cpp d96ed16
Kyle Hill Fix memory leak in Util.cpp f051f41
Kyle Hill Fix memory leak in AirPlayServer.cpp 997375d
Kyle Hill Fix use of uninitialized data in AirPlayServer.cpp 8ab6afe
Kyle Hill Fix memory leak in VTPFile.cpp b0fc451
Kyle Hill Fix virtual override in DummyVideoPlayer 7c59cdb
Kyle Hill Fix possible dereference of invalid iterator in RarManager.cpp 1dbb3a2
Kyle Hill Remove unused assignment in cddb.cpp 03ff853
Kyle Hill Fix use of invalid iterator in ZerconfBrowserAvahi b91cfe4
Kyle Hill Check return value of lseek when passed back to IoSupport.cpp 4058056
Kyle Hill Check return of stat64 from XFileUtils.cpp 70ce37d
Kyle Hill Fix memory leak in Application.cpp 4e4e836
Kyle Hill Fix uninitialized member in ZeroconfBrowser 5faa9db
Kyle Hill Fix possible use of uninitialized array in XBMChttp.cpp b0e4c9e
Kyle Hill Compile out code in NetworkInterfaceChanged
Instead of returning immediately form NetworkInterfaceChanged, compile
the body out to save some code space and maybe come compile time.
df640e3
Kyle Hill Fix reversed NULL check in GUIDialogSettings.cpp 1a874ea
Kyle Hill Fix possible NULL pointer dereference in GUIDialogAddonSettings cc0e59d
Kyle Hill Add missing break in GUIViewStateVideo.cpp 4c4260a
Kyle Hill Add missing breaks to GUIWindowSettingsCategory.cpp 0760464
Kyle Hill Fix possible NULL pointer dereference in TuxBoxUtil.cpp f70d2b8
Kyle Hill Fix possible NULL pointer dereference in DownloadQueueManager.cpp
It looks like it's possible for the following assert to fail and for
this function to return a NULL pointer, but at least we won't
dereference a NULL pointer before tripping the assert.
8cf3da6
Kyle Hill Fix possible NULL pointer dereferences in LastFMDirectory.cpp 2211049
Kyle Hill Fix mixup between Empty and empty in CharsetConverter.cpp
Empty() in StdString.h actually clears the string, while empty() in
std::string simply checks that the string length is greater than 0.
11f910f
Kyle Hill Fix unreachable database closes in XBMChttp.cpp 107ec43
Kyle Hill Remove unreachable code from emu_msvcrt.cpp 847f8f7
Kyle Hill Fix use of uninitialized members in AERemap.cpp 6484c3e
Kyle Hill Fix use of uninitialized data in SoftAE.cpp a97cff0
Kyle Hill Remove invalid semicolon from AESinkALSA.cpp 236f530
Kyle Hill Fix memory leak on read failure in APEv2Tag.cpp 80afa2e
Kyle Hill Fix possible memory leak in emu_kernel32.cpp bf4f248
Kyle Hill Fixed memory leak in coff.cpp be75bcc
Kyle Hill Fix file handle leak and buffer over/underrun
Fix possible file handle leak in AESinkOSS.cpp
Fix possible buffer underrun in Util.cpp
Fix possible buffer overrun in Util.cpp
Remove unused GetXBEPath() from IoSupport
426dadc
Kyle Hill Fix use of pointer to freed internals of a temporary fb40577
Kyle Hill Remove unneeded NULL check in LastFMDirectory.cpp ece42b6
Kyle Hill Fix use of uninitialized data in cdioSupport.cpp (again) f196a57
Kyle Hill Make vprepare pure virtual in dataset.h/.cpp 19ec82b
Kyle Hill Add missing breaks to DAAPFile.cpp b29388a
Kyle Hill Fixed missing break in GUIDialogKeyboardGeneric.cpp 4247fb6
Kyle Hill Fix possible divide-by-zero error in SoftAE.cpp a454c22
Kyle Hill Fix possible NULL pointer dereference in iso9660.cpp 03f44f3
Kyle Hill Remove unneeded NULL checks against arrays 4f03ae6
Kyle Hill Fix possible buffer overwrite in karaokelyricstextkar.cpp c26d1d1
Kyle Hill Fix non-NULL terminated string in XLCDproc.cpp d55768f
Kyle Hill Fix possible buffer overflow in SmbFile.cpp 24fd936
Kyle Hill Fix possible integer overflows 52a16dc
Kyle Hill Fix possible NULL pointer dereference in DNSNameCache.cpp b9a5cbc
Kyle Hill Fix possible NULL pointer dereference in DirectoryNode.cpp 740fdca
Kyle Hill Fix NULL pointer dereference and clean up #ifdefs in emu_kernel32.cpp 88c7649
Kyle Hill Fix possible NULL pointer dereference in CDDARipper.cpp eeb7ff3
Kyle Hill Check return of dynamic_cast in CDDARipJob.cpp 7df5d9c
Kyle Hill Fix possible divide-by-0 in WinSystemX11.cpp 1b56637
Kyle Hill Fix possible divide-by-0 in CDDACodec.cpp 8e5337c
Kyle Hill Fix up problems related to unreachable code
Most of these were harmless, but it looks like there was a real bug in
ZipFile.cpp.
595e57f
Kyle Hill Fix off-by-one errors bb335b2
Kyle Hill Check return from mkdir in SmbFile.cpp 0df52b1
Kyle Hill Check returns from fseek in coff.cpp 667cb2e
Kyle Hill Fix possible string non-terminiation in pyutil.cpp a73f215
Kyle Hill Check return values in XFileUtils.cpp bda8688
Kyle Hill Check return of sysconf() in ldt_keeper.c
Also fix up file formatting.  This was pretty bad :)
311ac37
Kyle Hill Fix checks for file descriptors in AESinkOSS.cpp 7906b8d
Kyle Hill Add check for invalid file descriptor in CDDARipJob.cpp e1f5392
Kyle Hill Fix confusion between empty() and clear() in LastFmManager.cpp 06cb22f
Kyle Hill Handle iterator error in DBusUtil.cpp 9da640c
Kyle Hill Check for return values in several files 2ff9f00
Kyle Hill Check return values in KeyboardStat.cpp e93b58e
Kyle Hill Fix missing return value checks in MultiPathDirectory.cpp and Reposit…
…ory.cpp
49f5af5

@elupus elupus was assigned Sep 4, 2012

Member

Voyager1 commented Sep 5, 2012

just a general comment. I don't feel comfortable throwing in all these changes after static coverity analysis. As it says, it's static analysis, so you need to complement this with dynamic testing :-)

So please this time please perform some basic functionality tests before merging... previous 'fixes' blew up dvdplayeraudio - see c1950f5

Member

elupus commented Sep 5, 2012

It blew up due changes I requested :-), failure on my part mainly.

It needs both review and testing obviously.

Member

kylhill commented Sep 6, 2012

I tested this for a few hours this evening doing typical usage scenarios: video playback, scanning for new data, viewing weather info, playing with settings and I didn't notice any issues.

I can't cover all of the possible code paths this PR hits, but I don't think any major functionality has regressed due to these changes.

Member

kylhill commented Sep 6, 2012

So... I was attempting to upload a new changeset to this PR and I pushed to "upstream" instead of "origin" in my repo. Since I have push privileges, these changes are now in mainline.

I'm really sorry that my mistake has resulted in this possibly premature merge. I will close this PR and follow up immediately on any feedback given on the changes that are already in mainline.

Again, sorry any headaches this may cause.

@kylhill kylhill closed this Sep 6, 2012

Member

Voyager1 commented Sep 6, 2012

Kyle - there's an access violation upon xbmc exit, at delete m_videoInfoScanner;

Member

kylhill commented Sep 6, 2012

Please see my comments on the commit in mainline. It looks like a merge conflict that git didn't tell me about.

Owner

Montellese commented Sep 6, 2012

Btw was it a forced push? Because then we'd need to check whether you overwrote some previous commits or not.

@ghost

ghost commented Sep 6, 2012

we really need to stop this from happening.

one idea would be that anyone not 100 comfortable with git volunteer to
have their push to main repo removed and only work through pr's until such
a time they are certain this won't happen again. they can then request it
back and get it. the threat of a wtf award is just not enough.

personally i have upstream as a read-only remote on my working repo, which
is where i do the work if it involves more than a few commits. exactly to
ensure this does not happen. one more line to type to pull from my working
repo to the upstream writable repo before i push but then i know i cannot
arse up without really wanting to.

spiff

Member

kylhill commented Sep 6, 2012

No. I had just rebased from upstream/master, which is probably why upstream was still fresh in my mind.

Owner

Montellese commented Sep 6, 2012

@cptspiff that's exactly how I have my repos set up as well. I usually always work in my working repo (unless it's a very easy fix) and there I can't push to upstream either.

Member

kylhill commented Sep 6, 2012

That's a good idea. I'll make my upstream repo read-only

Contributor

DDDamian commented Sep 6, 2012

Guess the comment comes a little late but a quick review of any AE-related commits are all good, and despite the force-push this has been a worthwhile effort Kyle. The odd blip (readily fixed) was worth it :)

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