After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 675888 - fix problems detected by cppcheck and clang static analysis
fix problems detected by cppcheck and clang static analysis
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
3.4.x (obsolete)
Other Linux
: Normal minor
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-05-11 15:12 UTC by Lubomír Sedlář
Modified: 2012-05-21 10:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Bugs found by cppcheck (3.06 KB, text/plain)
2012-05-11 15:12 UTC, Lubomír Sedlář
  Details
Fix a small memory leak (732 bytes, patch)
2012-05-11 15:15 UTC, Lubomír Sedlář
needs-work Details | Review
Fix another small memory leak (1.01 KB, patch)
2012-05-12 13:47 UTC, Lubomír Sedlář
needs-work Details | Review
Remove checks for negative guint (1.05 KB, patch)
2012-05-14 13:22 UTC, Lubomír Sedlář
committed Details | Review
Prevent descriptor leak when mozilla bookmarks file is incorrect (1.05 KB, patch)
2012-05-14 13:25 UTC, Lubomír Sedlář
needs-work Details | Review
Make sure not to read uninitialized memory (808 bytes, patch)
2012-05-14 13:27 UTC, Lubomír Sedlář
committed Details | Review
Don't leak mime_description (938 bytes, patch)
2012-05-15 14:39 UTC, Lubomír Sedlář
needs-work Details | Review
Cleanup if bookmarks import fails (1.81 KB, patch)
2012-05-15 14:44 UTC, Lubomír Sedlář
accepted-commit_now Details | Review
Cleanup if bookmarks import fails (1.85 KB, patch)
2012-05-19 17:35 UTC, Lubomír Sedlář
committed Details | Review
Don't leak mime_description (1.69 KB, patch)
2012-05-19 17:40 UTC, Lubomír Sedlář
rejected Details | Review
Fix another small memory leak (1.43 KB, patch)
2012-05-19 17:46 UTC, Lubomír Sedlář
committed Details | Review

Description Lubomír Sedlář 2012-05-11 15:12:44 UTC
Created attachment 213872 [details]
Bugs found by cppcheck

Running Cppcheck or Clang static analyzer of Epiphany source detects many possible problems. Even though most of them are insignificant, some memory leaks are detected as well. I would be happy to fix them.

The results of Cppcheck are attached, Clang output is slightly bigger though, so I'm only going to link to it:
http://www.fi.muni.cz/~xsedlar3/epiphany-clang/

Please note that the analysis was performed on Epiphany 3.4.1-1 obtained from Debian Unstable repository.
Comment 1 Lubomír Sedlář 2012-05-11 15:15:32 UTC
Created attachment 213873 [details] [review]
Fix a small memory leak

If decide_action_from_mime() fails to detect the MIME type, it dupes a string into a variable. The string is never read nor free'd in the function.
Comment 2 Lubomír Sedlář 2012-05-12 13:47:41 UTC
Created attachment 213918 [details] [review]
Fix another small memory leak
Comment 3 Lubomír Sedlář 2012-05-14 13:22:20 UTC
Created attachment 213997 [details] [review]
Remove checks for negative guint
Comment 4 Lubomír Sedlář 2012-05-14 13:25:02 UTC
Created attachment 213999 [details] [review]
Prevent descriptor leak when mozilla bookmarks file is incorrect

During error handling it was possible to escape a function without freeing some memory and closing mozilla bookmarks file.
Comment 5 Lubomír Sedlář 2012-05-14 13:27:05 UTC
Created attachment 214000 [details] [review]
Make sure not to read uninitialized memory

In popup_cmd_bookmark_link it was possible to read unitialized string. This patch initializes it to NULL.
Comment 6 Claudio Saavedra 2012-05-15 13:17:44 UTC
Review of attachment 213873 [details] [review]:

The mime_description is also leaked earlier in the function, I think that the earlier needs to be removed as well.
Comment 7 Claudio Saavedra 2012-05-15 13:21:34 UTC
Review of attachment 213918 [details] [review]:

::: embed/ephy-web-view.c
@@ +302,1 @@
 

Since location is not used until later on, wouldn't it be cleaner to set it right before it's used, instead?
Comment 8 Claudio Saavedra 2012-05-15 13:23:19 UTC
Review of attachment 213997 [details] [review]:

Agreed.
Comment 9 Claudio Saavedra 2012-05-15 13:28:17 UTC
Review of attachment 213999 [details] [review]:

::: src/bookmarks/ephy-bookmarks-import.c
@@ +764,1 @@
 

Not a big fan of replicating the cleanup code here, perhaps a goto out: pointing to the cleanup section and using  a boolean for the return value (initialized to TRUE and only set to FALSE here) would be better.
Comment 10 Claudio Saavedra 2012-05-15 13:30:57 UTC
Review of attachment 214000 [details] [review]:

Makes sense.
Comment 11 Lubomír Sedlář 2012-05-15 14:39:43 UTC
Created attachment 214103 [details] [review]
Don't leak mime_description

The first value assigned to mime_description is read in a condition, so this patch frees the memory after that.
Comment 12 Lubomír Sedlář 2012-05-15 14:44:57 UTC
Created attachment 214108 [details] [review]
Cleanup if bookmarks import fails

Instead of duplicating cleanup functions, this code uses goto with boolean variable.

I also moved creating name and url strings after opening file, so as not to leak them when opening fails.
Comment 13 Claudio Saavedra 2012-05-16 09:03:12 UTC
Review of attachment 214103 [details] [review]:

::: embed/ephy-download.c
@@ +220,1 @@
 

mime_description is not used at all in this function. It can safely be removed, so this patch is not addressing the root issue. Just remove it entirely.
Comment 14 Claudio Saavedra 2012-05-16 09:06:41 UTC
Review of attachment 214108 [details] [review]:

::: src/bookmarks/ephy-bookmarks-import.c
@@ +756,3 @@
 			}
 
+			if (node == NULL) {

Since there was a g_return_if_fail() here, perhaps you should add a g_warning() here. Otherwise the patch looks good, please commit.
Comment 15 Lubomír Sedlář 2012-05-19 17:35:38 UTC
Created attachment 214444 [details] [review]
Cleanup if bookmarks import fails

If `node' is NULL, issue a warning.
Comment 16 Lubomír Sedlář 2012-05-19 17:40:13 UTC
Created attachment 214446 [details] [review]
Don't leak mime_description

Completely remove mime_description string. Also remove a comment that referenced this variable, as it no longer makes much sense.

I don't have any commit rights for Epiphany, so I'm not able to commit the patches myself.
Comment 17 Lubomír Sedlář 2012-05-19 17:46:33 UTC
Created attachment 214448 [details] [review]
Fix another small memory leak

Initialize location right before use so that it can be free'd only in one place.
Comment 18 Claudio Saavedra 2012-05-21 10:54:54 UTC
I didn't commit the attachment 21446 [details] [review], as I noticed that the function is actually using
the variable but it's a total mess. I will open a new bug to track that one. The other
patches were landed, but for 214444 I did a few minor style changes.

Thank you!