[RFC][IcedTea-Web]: Add Logging of applet exceptions.

[Omair Majid]

On 01/11/2011 09:46 PM, Andrew Su wrote:
> Thanks for looking it over, here is the updated patch with the said
> changes.

I have added some comments in-line.

> diff -r dc02a605f905 netx/net/sourceforge/jnlp/NetxPanel.java
> --- a/netx/net/sourceforge/jnlp/NetxPanel.java	Fri Jan 07 08:00:08 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/NetxPanel.java	Tue Jan 11 21:45:49 2011 -0500
;
> @@ -68,6 +69,12 @@
>           super.run();
>       }
>
> +    @Override
> +    protected void showAppletException(Throwable t) {
> +        AppletLog.log(t); // log the exception

Please remove this in-line comment. It doesnt add anything that's isn't 
already clear. Perhaps a function level comment explaining what types of 
exceptions are this function logs might be better.

> +        super.showAppletException(t);
> +    }
> +
>       //Overriding to use Netx classloader. You might need to relax visibility
>       //in sun.applet.AppletPanel for runLoader().
>       protected void runLoader() {
> diff -r dc02a605f905 netx/net/sourceforge/jnlp/debug/AppletLog.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/debug/AppletLog.java	Tue Jan 11 21:45:49 2011 -0500
> @@ -0,0 +1,48 @@
> +package net.sourceforge.jnlp.debug;
> +
> +import java.io.ByteArrayOutputStream;
> +import java.io.IOException;
> +import java.io.PrintStream;
> +import java.util.logging.FileHandler;
> +import java.util.logging.Level;
> +import java.util.logging.Logger;
> +import java.util.logging.XMLFormatter;
> +
> +/**
> + * This class writes log information to file.
> + * @author Andrew Su (asu at redhat.com,andrew.su at utoronto.ca)
> + *
> + */
> +public class AppletLog extends Log {

Is there a reason this has to be public? I recommend moving this class 
(and Log) to net.sourceforge.jnlp package and making it package-private.

> +    public static Logger logger;

Please avoid using public static objects. This is likely to lead to 
security issues. An untrusted applet may be able to access this logger 
and grab the stream, reading potentially sensitive information.

> +
> +    static {
> +        try {
> +            // If logging is enabled, we create logger.
> +            if (enableLogging) {
> +                String fn = icedteaLogDir + "Plugin" + java.lang.System.currentTimeMillis() + ".log";

If I understand this correctly, every applet creates a new log file. Is 
there anything that cleans logs up? You may want to add a FIXME or a 
TODO here noting this. Is there any specific reason you use "Plugin" as 
opposed to "plugin"?

> +                boolean append = false;
> +                FileHandler fh = new FileHandler(fn, append);
> +                fh.setFormatter(new XMLFormatter());
> +                String logClassName = AppletLog.class.getName();
> +                logger = Logger.getLogger(logClassName);
> +                logger.setLevel(Level.ALL);
> +                logger.addHandler(fh);
> +            }
> +        } catch (IOException e) {
> +            e.printStackTrace();

If an exception occurs during setup of logging, shouldn't logging be 
disabled? I am not sure what happens here if fh = new FileHandler() 
throws an exception.

> diff -r dc02a605f905 netx/net/sourceforge/jnlp/debug/Log.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/debug/Log.java	Tue Jan 11 21:45:49 2011 -0500
> @@ -0,0 +1,43 @@
> +package net.sourceforge.jnlp.debug;
> +
> +import java.io.File;
> +
> +import net.sourceforge.jnlp.config.DeploymentConfiguration;
> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +
> +/**
> + * This file provides the information required to do logging.
> + * @author Andrew Su (asu at redhat.com,andrew.su at utoronto.ca)
> + *
> + */
> +public class Log {

If you can, please make this class non-public.

> +
> +    // Directory where the logs are stored.
> +    public static String icedteaLogDir;
> +

Please make these fields private (or protected). An attacker might be 
able to figure out $HOME from this log dir.

> +    public static boolean enableLogging = false;
> +    public static boolean enableTracing = false;
> +

[snip]

> +    Log() {}
> +

If you dont want this class to be initialized, perhaps make it abstract?

Cheers,
Omair