findbugs: Possible null pointer dereference warning

Question

I got Possible null pointer dereference in convertMultiPartToFile(MultipartFile) due to return value of called method [line 91] in my findbugs report.

Here is the code:

private File convertMultiPartToFile(MultipartFile file) throws IOException {
        //line below is the line 91
        if (file == null || file.getOriginalFilename() == null)
            throw new InputValidationException("fileNameInvalid", i18n, file.getOriginalFilename());
        File convFile = new File(file.getOriginalFilename());
        FileOutputStream fos = new FileOutputStream(convFile);
        fos.write(file.getBytes());
        fos.close();
        return convFile;
}

I already check the null value of the file, why do I still get the warning?

Update 1:

After I removed the file name in the exception, it still has a warning on the line below.

private File convertMultiPartToFile(MultipartFile file) throws IOException {           
        if (file == null || file.getOriginalFilename() == null)
            throw new InputValidationException("fileNameInvalid", i18n, "");
        File convFile = new File(file.getOriginalFilename()); // warning here
        FileOutputStream fos = new FileOutputStream(convFile);
        fos.write(file.getBytes());
        fos.close();
        return convFile;
}

Update 2:

private File convertMultiPartToFile(MultipartFile file) throws IOException {
    File convFile = null;
    if (file != null && file.getOriginalFilename() != null) {
        convFile = new File(file.getOriginalFilename()); //line 91
        try (FileOutputStream fos = new FileOutputStream(convFile);) {
            fos.write(file.getBytes());
        }
    }
    return convFile;
}

Adapted @Michael Peacock's answer, the warning is still there.

Possible null pointer dereference in convertMultiPartToFile(MultipartFile) due to return value of called method

Bug type NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE (click for details) In class com.corpobids.server.service.AwsAPIService In method convertMultiPartToFile(MultipartFile)

Local variable stored in JVM register ?

Method invoked at Service.java:[line 91]

Known null at Service.java:[line 91]

You probably meant to write `file == null`, not `file != null` — yole, Jul 26 '18 at 13:50
If `file` is null, you'll get an NPE inside your `throw` statement when you call `file.getOriginalFilename()`. — yole, Jul 26 '18 at 13:52

score 2 · Accepted Answer · edited Jul 20 '21 at 15:12

2

There are a couple things here. First, you need to guarantee that you're going to close the FileOutputStream. This is done differently depending on the JDK you're using. Prior to JDK 1.7, you would use a finally block to close the fos. From JDK 1.7 forward, use a try with resources.

In addition, only proceed with the file processing if there's something to process. I haven't tested this code, but this should work to eliminate the possible NPE. Note how we've flipped the condition so that we skip processing the file if we can.

JDK <= 1.6:

private File convertMultiPartToFileJDK16(MultipartFile file) throws IOException {           

    File convFile = null;
    FileOutputStream fos = null;

    if (file != null && file.getOriginalFilename() != null) {
        try {
            String originalFilename = file.getOriginalFilename();
            if (originalFilename != null) { 
               convFile = new File(originalFilename); 
               fos = new FileOutputStream(convFile);
               fos.write(file.getBytes());

        }
        catch(IOException ex){
                // handle IOException or rethrow it
        }
        finally {
            fos.close();
        }
    }
    return convFile;
}

JDK >= 1.7:

    private File convertMultiPartToFileJDK17(MultipartFile file) throws IOException {
    File convFile = null;
    if (file != null ) {

        String originalFilename = file.getOriginalFilename();

        if (originalFilename != null) {
            convFile = new File(originalFilename);

            try(FileOutputStream fos = new FileOutputStream(convFile);) {
                fos.write(file.getBytes());
            }
        }
    }
    return convFile;
}

edited Jul 20 '21 at 15:12

arpit sharma

41
7

answered Jul 26 '18 at 15:16

Michael Peacock

2,011
1
11
14

Still get the warning. – Cosaic Jul 26 '18 at 15:42
Edited - it turns out that calling file.getOriginalFilename() can return null, so you should check that before using it to create the new file. See this post about the meaning of that findbugs warning - https://stackoverflow.com/questions/12242291/what-is-the-meaning-of-possible-null-pointer-dereference-in-findbug – Michael Peacock Jul 26 '18 at 15:53
With the edit this looks correct to me in terms of branch coverage (which is what FindBugs cares about). – John Stark Jul 26 '18 at 16:37
It does fix the warning, but I do not know why. Does it actually have quality improvement? `String originalFilename = file.getOriginalFilename(); if (originalFilename != null)` seems still a duplicate check to me. – Cosaic Jul 26 '18 at 20:25
The original issue occurred because findbugs thinks it's potentially possible to call "new File(null)" when you call new File(file.getOriginalFilename()). You had already guarded against this, but findbugs doesn't know that, so it throws the warning. The relevant part of the link I provided above is, "Of course, the problem might be that the branch or statement is infeasible and that the null pointer exception can't ever be executed; deciding that is beyond the ability of FindBugs." – Michael Peacock Jul 26 '18 at 21:29
@MichaelPeacock So in practice should I add it to warning suppress? – Cosaic Jul 30 '18 at 13:09
Hi @Cosaic - I don't usually advocate suppressing warnings since they're usually there for a good reason. Here, Findbugs just happened to provide a misleading warning, but that hardly means that the warning would be misleading in all cases. I would not suppress the warning, and review the identified issues on a case by case basis. – Michael Peacock Aug 01 '18 at 13:16

score 0 · Answer 2 · answered Jul 26 '18 at 14:39

Edit: I think I was still thinking about this using file != null so this probably is incorrect. Let me know if breaking out the conditions separately doesn't fix the warning and I'll update/remove this answer.

Original: The or condition will short-circuit as soon as any condition evaluates to true. In this case, you could have a non-null MultipartFile object, whose `file.getOriginalFilename()' evaluates to null.

As soon as the conditional statement determines that file != null it's going to skip the throw clause and drop down to the next line. At which point, you would be passing null into the File() constructor (again, assuming you have a non-null file, but a null originalFilename field).

The FindBugs module doesn't like the idea of passing a possibly dereferenced value into the File() constructor. It would be safer to evaluate the conditions separately and act accordingly in each:

if (file == null) {
    throw new InputValidationException("fileObjectIsNull", i18n, "")
}

if (file.getOriginalFilename() == null) {
    throw new InputValidationException("fileNameInvalid", i18n, "");
}

File convFile = new File(file.getOriginalFilename());

findbugs: Possible null pointer dereference warning

2 Answers2