[rfc][icedtea-web] C++-side small fixes (RH 1121549)
Andrew Azores
aazores at redhat.com
Tue Jul 22 15:09:24 UTC 2014
On 07/22/2014 05:22 AM, Jiri Vanek wrote:
> 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)".
This one didn't seem like it really needed to be fixed, it's in
"test_main" and the actual useful result is stored in 'a3' anyway. The
return value is just a boolean if the read was successful.
>
>
> 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.
Does this one matter? I added a close() anyway but like Omair says it
seems like it will be cleaned up anyway. But it's better to explicitly
clean it I suppose.
>
> 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
Those were just duplicates due to java_result being redeclared, so
they're taken care of with that fix.
> 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
>
>
This one is addressed since now the startup_error may be returned (see
below).
>
> 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?
Can it? Looking through start_jvm_if_needed, all the cases I've seen
that return other than NPERR_NO_ERROR look like conditions where we
would fail to start, but I could very well be wrong about that.
>>
>> // 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.
>
So all of this in a separate changeset right? Can't most of this work be
done just by checking errno after mkdir fails?
>
>> }
>> 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.
Yea, I couldn't find a reason for it to. Seems like it was probably just
an accident.
Thanks,
--
Andrew A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rh1121549-fixes-2.patch
Type: text/x-patch
Size: 3789 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140722/d1542831/rh1121549-fixes-2.patch>
More information about the distro-pkg-dev
mailing list