[RFC][icedtea-web:netx]: handle error code returning functions

[Omair Majid]

On 03/09/2011 05:35 PM, Denis Lila wrote:
> Hi.
>
> findbugs found 10 cases where we call methods such as
> File.delete() and File.mkdirs() that can fail without
> throwing exceptions and we don't handle the return
> value that indicates error. The attach patch attempts
> to fix that.
>
> I haven't tested it yet. I will do so tomorrow. I just
> wanted to get some feedback on the way I'm handling things.
>

I do have a few concerns noted inline below.

> diff -r d91d02e798ff netx/net/sourceforge/jnlp/Launcher.java
> --- a/netx/net/sourceforge/jnlp/Launcher.java	Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/Launcher.java	Wed Mar 09 17:34:52 2011 -0500
> @@ -709,7 +709,9 @@
>               File netxRunningFile = new File(JNLPRuntime.getConfiguration()
>                       .getProperty(DeploymentConfiguration.KEY_USER_NETX_RUNNING_FILE));
>               if (!netxRunningFile.exists()) {
> -                netxRunningFile.getParentFile().mkdirs();
> +                if (!netxRunningFile.getParentFile().mkdirs()) {
> +                    throw new IOException(R("RCantCreateDir", netxRunningFile.getParentFile()));
> +                }
>                   FileUtils.createRestrictedFile(netxRunningFile, true);
>                   FileOutputStream fos = new FileOutputStream(netxRunningFile);
>                   try {

You might want to change this (and other similar pieces of code) to:

if (!netxRunningFile.getParentFile.isDir() && 
!netxRunningFile.getParentFile.mkdirs()) {

To ensure it deals correctly with mkdirs() failing because the directory 
already exists.

> diff -r d91d02e798ff netx/net/sourceforge/jnlp/services/XPersistenceService.java
> --- a/netx/net/sourceforge/jnlp/services/XPersistenceService.java	Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/services/XPersistenceService.java	Wed Mar 09 17:34:52 2011 -0500
> @@ -16,6 +16,8 @@
>
>   package net.sourceforge.jnlp.services;
>
> +import static net.sourceforge.jnlp.runtime.Translator.R;
> +
>   import java.io.*;
>   import java.net.*;
>   import java.util.*;
> @@ -93,11 +95,14 @@
>           checkLocation(location);
>
>           File file = toCacheFile(location);
> -        file.getParentFile().mkdirs();
> +        if (!file.getParentFile().mkdirs()) {
> +            throw new IOException(R("RCantCreateDir", file.getParentFile()));
> +        }
>

This might not be safe. XPersistenceService is fully privileged code 
invoked by untrusted code. Propagating any sensitive value back (by 
writing it out somewhere or using it in the exception message) could 
lead to information leaks.

file is always located under user.home; we dont want untrusted code to 
figure out what user.home is based on the exception message. A more 
generic error message that does not contain the path might be better.

>           if (file.exists())
>               throw new IOException("File already exists.");
>
> +        // the file creations should fail if mkdirs() fails.
>           FileUtils.createRestrictedFile(file, true);
>
>           return maxsize;
> @@ -110,7 +115,10 @@
>       public void delete(URL location) throws MalformedURLException, IOException {
>           checkLocation(location);
>
> -        toCacheFile(location).delete();
> +        File tocache = toCacheFile(location);
> +        if (!tocache.delete()) {
> +            System.err.println(R("RCantDeleteFile", tocache));
> +        }

Likewise.

>       }
>
>       /**
> @@ -124,7 +132,9 @@
>           if (!file.exists())
>               throw new FileNotFoundException("Persistence store for "
>                       + location.toString() + " is not found.");
> -        file.getParentFile().mkdirs();
> +        if (!file.getParentFile().mkdirs()) {
> +            throw new IOException(R("RCantCreateDir", file.getParentFile()));
> +        }
>

And again.

Cheers,
Omair