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

[Andrew Su]

----- Original Message -----
> From: "Omair Majid" <omajid at redhat.com>
> To: "Andrew Su" <asu at redhat.com>
> Cc: "OpenJDK" <distro-pkg-dev at openjdk.java.net>
> Sent: Monday, January 17, 2011 11:06:36 AM
> Subject: Re: [RFC][IcedTea-Web]: Add Logging of applet exceptions.
> 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.
Removed + fixed.

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

I am happy with the recommendation, changed.


>
> > + 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.
Changed to private.

>
> > +
> > + 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.
No, static initialization blocks are called only once when loading the 
class.
Unless they kill the plugin then reload another page it will use the 
same file.

> Is
> there anything that cleans logs up? You may want to add a FIXME or a
> TODO here noting this.
As of this moment, no. But can be added to itw-settings to clean the 
logs. (Similar to how we clean cache)

> Is there any specific reason you use "Plugin"
> as
> opposed to "plugin"?
The naming was a typo, meant to be lower case. Fixed.


>
> > + 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.
Added condition on whether logger is null, if logger is null, that means 
we were not successful in getting the filehandle.

>
> > 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.
I've left it at package-level.

>
> > +
> > + // 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?
Ah, slipped my mind. Changed to abstract.

Regards,
   Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20110117_add_package_to_makefile_v5.patch
Type: text/x-patch
Size: 4434 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110117/85fbb8d0/20110117_add_package_to_makefile_v5.patch