GNOME Bugzilla – Bug 675888
fix problems detected by cppcheck and clang static analysis
Last modified: 2012-05-21 10:58:16 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.
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.
Created attachment 213918 [details] [review] Fix another small memory leak
Created attachment 213997 [details] [review] Remove checks for negative guint
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.
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.
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.
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?
Review of attachment 213997 [details] [review]: Agreed.
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.
Review of attachment 214000 [details] [review]: Makes sense.
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.
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.
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.
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.
Created attachment 214444 [details] [review] Cleanup if bookmarks import fails If `node' is NULL, issue a warning.
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.
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.
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!