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

[Jiri Vanek]

On 07/21/2014 04:07 PM, Andrew Azores wrote:
> Hi,
>
> 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 ;)


not at all. Moreover good work!

 From globals cope I'm missing the fix for

Defect type: CHECKED_RETURN
7. icedtea-web-1.5/plugin/icedteanp/IcedTeaParseProperties.cc:328:unchecked_value – No check of the 
return value of "read_deploy_property_value(std::string("deployment.security.level", 
std::allocator<char>()), a3)".


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.

and family of

1. Defect type: CLANG_WARNING
1. icedtea-web-1.5/plugin/icedteanp/IcedTeaScriptablePluginObject.cc:750:9:warning – Dereference of 
undefined pointer value
Expand
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
Expand
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



Anyway they can be handled in another changest (I can run custom covescan for you once this is 
pushed to see)

Some nits inline:
>
> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1121549
>
> Thanks,
>
> --
> Andrew A
>
>
> rh1121549-fixes.patch
>
>
> diff --git a/plugin/icedteanp/IcedTeaNPPlugin.cc b/plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc
> @@ -375,6 +375,10 @@ ITNP_New (NPMIMEType pluginType, NPP ins
>
>     // start the jvm if needed
>      NPError startup_error = start_jvm_if_needed();
> +   if (startup_error != NPERR_NO_ERROR) {
> +	   PLUGIN_ERROR ("Failed to start JVM\n");
> +	   return startup_error;
> +   }

Cant this exit when it should not?
>
>     // Initialize data->instance_id.
>     //
> @@ -2027,7 +2031,7 @@ NP_Initialize (NPNetscapeFuncs* browserT
>     NPError np_error =  initialize_data_directory();
>     if (np_error != NPERR_NO_ERROR)
>       {
> -      PLUGIN_ERROR("Unable create data directory %s\n");
> +      PLUGIN_ERROR("Unable to create data directory %s\n", data_directory.c_str());
>         return np_error;
>       }

can go in.
>
> 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;
>   		}
> @@ -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());
> +			}
>   		}
>   		return r2;


For those 4 mkdirs - if this directory already exists then the mkdir should not be called at all 
(maybe some check for its write ability?? moreover 100% for different changeset)
Anyway. THis check is included. So I woud liek to add
(!IcedTeaPluginUtilities::file_exists(r1){} if (!isWriteableDirectory(){"warning to user}

As thsoe are called only when  and it do not exists, then more serious warring with possible 
workarounds like check permissions on parents,  set XDG_CONFIG* for first hunk and.. not sure what 
for second :) or similar and warning like ITW will not work., SHould be printed.


>   	}
> diff --git a/plugin/icedteanp/IcedTeaScriptablePluginObject.cc b/plugin/icedteanp/IcedTeaScriptablePluginObject.cc
> --- a/plugin/icedteanp/IcedTeaScriptablePluginObject.cc
> +++ b/plugin/icedteanp/IcedTeaScriptablePluginObject.cc
> @@ -704,7 +704,7 @@ IcedTeaScriptableJavaObject::setProperty
>                       browser_functions.intfromidentifier(name_id) >= 0) // else if array and requesting index
>           {
>
> -            JavaResultData* java_result = java_request.getArrayLength(instance_id);
> +            java_result = java_request.getArrayLength(instance_id);
>               if (java_result->error_occurred)
>               {
>                   PLUGIN_ERROR("ERROR: Couldn't fetch array length\n");
>


I have looked into code and have not found a reason for in-the-middle-decklaration. So this is 
really probably fix for hidden minor bug.


J.