[rfc][icedtea-web] C++-side small fixes (RH 1121549)

[Omair Majid]

Hi,

* Jiri Vanek <jvanek at redhat.com> [2014-07-22 05:23]:
> On 07/21/2014 04:07 PM, Andrew Azores wrote:
> >This patch addresses the issues covered in [0]. I'm not so familiar with the C++ side and my C++ is
> >much much rustier than my Java, so this patch may be shockingly rough for its size ;)

Patch looks mostly okay to me. As Jiri says, though, it does not address
all issues in the bug you linked.

> 1. Defect type: RESOURCE_LEAK
> 10.
> icedtea-web-1.5/plugin/icedteanp/IcedTeaPluginUtils.cc:1154:leaked_handle –
> Handle variable "plugin_file_log_fd" going out of scope leaks the handle.

Meh, I can live with this. This log file is opened on startup and used as
long as the plugin process is live. So its lifetime is approximately
the lifetime of the entire process. The OS will clean up everything when
the process exits anyway.

> 1. Defect type: CLANG_WARNING
> 1. icedtea-web-1.5/plugin/icedteanp/IcedTeaScriptablePluginObject.cc:750:9:warning
> – Dereference of undefined pointer value

> 2. Defect type: CLANG_WARNING
> 1. icedtea-web-1.5/plugin/icedteanp/IcedTeaScriptablePluginObject.cc:728:13:warning
> – Value stored to 'java_result' is never read

> 3. Defect type: CLANG_WARNING
> 1. icedtea-web-1.5/plugin/icedteanp/IcedTeaNPPlugin.cc:377:12:warning –
> Value stored to 'startup_error' during its initialization is never read

>From what I can tell, these three should be addressed by the patch
already. These are duplicates, I believe.

> >diff --git a/plugin/icedteanp/IcedTeaParseProperties.cc b/plugin/icedteanp/IcedTeaParseProperties.cc
> >--- a/plugin/icedteanp/IcedTeaParseProperties.cc
> >+++ b/plugin/icedteanp/IcedTeaParseProperties.cc
> >@@ -133,10 +133,14 @@ string  get_log_dir(){
> >  			string r1= string(getenv ("XDG_CONFIG_HOME"))+"/icedtea-web";
> >  			string r2 = r1+"/"+default_itw_log_dir_name;
> >  			if (!IcedTeaPluginUtilities::file_exists(r1)){
> >-				g_mkdir(r1.c_str(), 755);
> >+				if (!g_mkdir(r1.c_str(), 755)) {
> >+					PLUGIN_ERROR("Failed to create directory %s\n", r1.c_str());
> >+				}
> >  			}
> >  			if (!IcedTeaPluginUtilities::file_exists(r2)){
> >-				g_mkdir(r2.c_str(), 755);
> >+				if (!g_mkdir(r2.c_str(), 755)) {
> >+					PLUGIN_ERROR("Failed to create directory %s\n", r2.c_str());
> >+				}
> >  			}
> >  			return r2;

PLUGIN_ERROR just logs errors, right? It seems like this code still
returns the unusable path `r2` in case of an error. Maybe an invalid
path should be indicated explicitly?

> >@@ -144,10 +148,14 @@ string  get_log_dir(){
> >  		string r1 = string(mypasswd->pw_dir)+"/.config/icedtea-web";
> >  		string r2 = r1+"/"+default_itw_log_dir_name;
> >  		if (!IcedTeaPluginUtilities::file_exists(r1)){
> >-			g_mkdir(r1.c_str(), 755);
> >+			if (!g_mkdir(r1.c_str(), 755)) {
> >+				PLUGIN_ERROR("Failed to create directory %s\n", r1.c_str());
> >+			}
> >  		}
> >  		if (!IcedTeaPluginUtilities::file_exists(r2)){
> >-			g_mkdir(r2.c_str(), 755);
> >+			if (!g_mkdir(r2.c_str(), 755)) {
> >+				PLUGIN_ERROR("Failed to create directory %s\n", r2.c_str());
> >+			}
> >  		}

Same issue here.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681