3
\$\begingroup\$

I have the following code which I am running through fortify. Why it gets marked for poor error handling, throw inside finally?

private String getResourceContent(String fileName) throws IOException {

    try (InputStream resource = ErrorResource.classLoader.getResourceAsStream(fileName)) {
        return new String(resource.readAllBytes(), StandardCharsets.UTF_8);
    } catch (NullPointerException n) {
        throw new ErrorDescriptorException(
                String.format("Error loading Error description data from Resource file [%s].", fileName), n);
    }
}
\$\endgroup\$
  • \$\begingroup\$ What is ErrorDescriptorException? \$\endgroup\$ – eric.m Aug 27 '19 at 11:24
  • \$\begingroup\$ class ErrorDescriptorException extends RuntimeException \$\endgroup\$ – Karan Khanna Aug 27 '19 at 11:24
6
\$\begingroup\$

ClassLoader.getResourceAsStream() throws a NullPointerException if the given name is null, something you can easily check. InputStream.readAllBytes() does not throw a NullPointerException, and neither does instatiating the String.

So basically, the null exception will happen if and only if fileName is null, or if the resource is not found and therefore resource is null; you can easily check both without catching an exception:

private String getResourceContent(String fileName) throws IOException {
    if (fileName == null) {
        // now what?
    }
    try (InputStream resource = ErrorResource.classLoader.getResourceAsStream(fileName)) {
        if (resource == null) {
            // now what?
        }
        return new String(resource.readAllBytes(), StandardCharsets.UTF_8);
    }
}

The question now is what you want to do if fileName or resource is null. This depends on how you want your function to be used; in my opinion, you should expect the user to give a file name that is not null, so throw a NullPointerException if that is not the case. In the resource case, you could use your custom exception for that.

Now, what if readAllBytes() throws an exception? You can either let it propagate to the caller (like it's happening now) or use ErrorDescriptorException as a wrapper (but maybe this makes fortify complain again):

private String getResourceContent(String fileName) throws ErrorDescriptorException {
    if (fileName == null) {
        throw new NullPointerException();
    }
    try (InputStream resource = ErrorResource.classLoader.getResourceAsStream(fileName)) {
        if (resource == null) {
            throw new ErrorDescriptorException(
                String.format("Error loading Error description data from Resource file [%s].", fileName));
        }
        return new String(resource.readAllBytes(), StandardCharsets.UTF_8);
    }
    catch (IOException | OutOfMemoryException e) {
        throw new ErrorDescriptorException(
                String.format("Error loading Error description data from Resource file [%s].", fileName), e);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ NPE can also be there at ErrorResource.classLoader.getResourceAsStream(fileName). ErrorResource.classLoader can be null if loaded by bootstrap classloader. \$\endgroup\$ – Karan Khanna Aug 27 '19 at 11:49
  • 2
    \$\begingroup\$ True, you should add a case for that one too. The idea here is that catching NullPointerException is generally a bad idea as you can usually just check if == null \$\endgroup\$ – eric.m Aug 27 '19 at 11:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.