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

[Jiri Vanek]

On 10/05/2015 08:08 PM, Jacob Wisor wrote:
> 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

no.

The crash was because of:
initFileLog calls:
get_log_dir which logs on error
and as addition get_log_dir calls create_dir which logs  on debug mode on.

The LAst one is the trouble maker, which caused segfault.when both file log and verbose/debug mode 
was on.


> 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.

On contrary, yes, I must admit I'm confused:) And hoping a bit that you will post na patch making 
various IO in the plugin better or logging safer.
>
> 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.

As I repeated many times, and hoefully highlighted in this emial's first paragraph, tnothing else 
then "says is that initFileLog() has been called" is wonted.
>

Tahnx!
   J.