[rfc][icedtea-web] logging to file before file is prepared patch

[Jacob Wisor]

On 09/29/2015 at 01:45 PM Jiri Vanek wrote:
> Hello!
>
> I have been noticed about segfault when both debuging anf filelogging is on.
>
> Issue is caused by printing of information into file before the file is actually
> prepared.
>
> The issue is presented since 1.4 but only 1.6 and head are affected - probably
> because thewy are logging somehting more. I would like to push it to 1.5 and up.
>
> Attached patch is disabling printing to file before the file is actually prepared.

I do not think this is the proper way to do it. You did not even bother to get 
to the root of the cause.

> diff -r b02ae452f99f plugin/icedteanp/IcedTeaPluginUtils.h
> --- a/plugin/icedteanp/IcedTeaPluginUtils.h	Thu Sep 24 16:32:30 2015 +0200
> +++ b/plugin/icedteanp/IcedTeaPluginUtils.h	Tue Sep 29 13:15:10 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;                                        \
>        }                                                                       \

The problem is that for any reason opening the log file may fail at any time, 
also writing to it may fail at any time. So first, you do not know the state of 
the log file after IcedTeaPluginUtilities::initFileLog() has returned. You just 
do not. You are just assuming that initialization of the log file _always_ works.
Second, it is better to check plugin_file_log for NULL before writing to it. So, 
things would rather look like this:
>     if (plugin_debug_to_file && plugin_file_log) {        \
And third, you should better check the return values of all those subsequent 
printfs because writing to files can fail at any time. Well, you /can/ ignore 
them but then you could also stop bothering calling any subsequent functions 
with this file after an error has occurred (because you know they are probably 
going to fail too).

Oh, and limiting a log message size to MESSAGE_SIZE is just wired, absolutely 
arbitrary, and not really necessary. Furthermore, combining the same message 
with snprintf multiple times for different output targets is also a waste of 
computing power.

Check this out:
IcedTeaNPPlugin.h:
> extern FILE * plugin_file_log;

IcedTeaPluginUtils.cc:
> void IcedTeaPluginUtilities::initFileLog(){
>     if (plugin_file_log != NULL ) {
>         //reusing
>         return;
>     }

The initialization of the log file is definitely wrong here: plugin_file_log is 
going to be of any possible value after loading the shared object into memory. 
It must have been just pure luck that plugin_file_log has been initialized to 
NULL after loading for any time logging to file from the shared object has ever 
worked. If you want to make sure the log file does not get reinitialized after 
it has been initialized then plugin_file_log must be explicitly initialized to 
NULL before making any assumptions whether to initialize the log file.

Besides, the variable name plugin_file_log is terrible, but this is an entirely 
different issue.

Regards,
Jacob