/hg/release/icedtea-web-1.6: Fixed possible segfault during file...

[Jacob Wisor]

On 10/05/2015 at 01:33 PM Jiri Vanek wrote:
>>>   static guint appletviewer_watch_id = -1;
>>>
>>>   bool debug_initiated = false;
>>> +bool file_logs_initiated = false;
>>
>> This exactly what I meant. There is no need for this. It does not add any
>> logic nor is it correct.
> How it can not be necessary? The methods in initFileLogs may log. And in debug
> mode the log always.  So they are crashing plugin.

The cause for the plug-in to crash was an uninitialized file pointer, namely 
plugin_file_log, not a missing additional variable. plugin_file_log did not get 
always properly initialized because initFileLog() assumed it to be always NULL 
before it has ever run. This was clearly a false assumption (because the value 
of an uninitialized variable in C/C++ is undefined). Now, that plugin_file_log 
is always initialized to NULL at load time, initFileLog()'s assumption has 
become correct. And, since initFileLog() now properly indicates the file log to 
have been initialized by setting plugin_debug_to_file to true, you do not need 
any additional logic or variables. If anything, always check the variable at 
stake, not a proxy variable before a function call.
Just please take some time to think about it. Do not haste and do not start 
slapping around new variables just to make the problem go seemingly away.

>> This makes me assume that you either did not read my post carefully enough or
>> did not understand it.
>
> Probably the second. Anyway following was spoken :
>  >> So I can proceed with original patch (+ above -FILE +FILe)? As sigsev was what
>  >> it was about to fix :)
>  >
>  > Yep, I think so.
>
> So I took it as green light. Sorry for misunderstanding.
>
>> Please remove this, as this does not add any program logic and is based on
>> wrong assumptions.
>
> Not unless better fix for "segfault because of logging during fileLogs
> initiation" is known.
>>
>>>   int plugin_debug = getenv ("ICEDTEAPLUGIN_DEBUG") != NULL;
>>>   bool plugin_debug_headers = false;
>>>   bool plugin_debug_to_file = false ;
>>>   bool plugin_debug_to_streams = true ;
>>>   bool plugin_debug_to_system = false;
>>>   bool plugin_debug_to_console = true;
>>> -FILE *  plugin_file_log;
>>> +FILE *  plugin_file_log = NULL;
>>>   std::string plugin_file_log_name;
>>>
>>>   int plugin_debug_suspend = (getenv("ICEDTEAPLUGIN_DEBUG") != NULL) &&
>>> diff -r 75504136acda -r cbc3174bed98 plugin/icedteanp/IcedTeaNPPlugin.h
>>> --- a/plugin/icedteanp/IcedTeaNPPlugin.h    Tue Sep 22 18:24:33 2015 +0200
>>> +++ b/plugin/icedteanp/IcedTeaNPPlugin.h    Fri Oct 02 15:35:30 2015 +0200
>>> @@ -117,6 +117,7 @@
>>>
>>>   // debug switches
>>>   extern bool debug_initiated;
>>> +extern bool file_logs_initiated;
>>>   extern int plugin_debug;
>>>   extern bool plugin_debug_headers;
>>>   extern bool plugin_debug_to_file;
>>> diff -r 75504136acda -r cbc3174bed98 plugin/icedteanp/IcedTeaPluginUtils.h
>>> --- a/plugin/icedteanp/IcedTeaPluginUtils.h    Tue Sep 22 18:24:33 2015 +0200
>>> +++ b/plugin/icedteanp/IcedTeaPluginUtils.h    Fri Oct 02 15:35:30 2015 +0200
>>> @@ -86,6 +86,7 @@
>>>         plugin_debug_to_console =
>>> is_java_console_enabled();                    \
>>>         if (plugin_debug_to_file)
>>> {                                             \
>>>
>>> IcedTeaPluginUtilities::initFileLog();                             \
>>> +           file_logs_initiated = true;                                        \
>>
>> Again, you cannot assume that after IcedTeaPluginUtilities::initFileLog() has
>> been called, the log
>> file has been properly initialized.
>
> But it des not meter. If they were not, then ITW will crash and it is OK.
>
> (better debugging of this should bring the patch which is checking the io returns)
>
> The fix with new variable is to avoid known reason of segfault - that ITW is
> trying to log to files, during the preparation of themselves.
> The logging in helper methods is good - they are reused several times.

Sorry, but you are obviously confused. The following two statements say nothing 
about the state of either the plugin_file_log file pointer nor the value of 
plugin_debug_to_file.

IcedTeaPluginUtilities::initFileLog();
file_logs_initiated = true;

At best, all it says is that initFileLog() has been called. No other effects can 
be assumed, not even the value of plugin_debug_to_file.

Regards,

Jacob