Ghidra: a potential NPE in DmgServer.java

[HermioneSW]

Describe the bug

1. In DmgFileReader.java (GPL/DMG/src/dmg/java/mobiledevices/dmg/reader/DmgFileReader.java), line 256 function getInfo returns null when path is null.

1. In DmgServer.java (GPL/DMG/src/dmg/java/mobiledevices/dmg/server/DmgServer.java) line 106 List<String> infoList = dmgFileReader.getInfo(path); will assign infoList null.

2. The expression infoList.size() in line 107 will NPE.

To Reproduce
See above

Expected behavior
N/A

Screenshots
N/A

Attachments
N/A

Environment (please complete the following information):
N/A

Additional context
N/A

[dev747368]

Is there anyway to trigger any of these possible NPEs via normal user interaction, or are they just poor coding waiting to bite in the future sometime?

[HermioneSW]

@dev747368 we are not running the software, the report is generated by a static analysis tool currently under research and development. It would be great if you can confirm or fix the bugs, which we believe at least for some are real.

Thanks a million!!!!!

[mumbel]

It would be great if you can confirm or fix the bugs, which we believe at least for some are real.

I think that's @dev747368 's point. Seems like the onus would be on you for that? A quick review of this bug, doesn't look like path can be NULL there

[HermioneSW]

It would be great if you can confirm or fix the bugs, which we believe at least for some are real.

I think that's @dev747368 's point. Seems like the onus would be on you for that? A quick review of this bug, doesn't look like path can be NULL there

Would you mind me asking why not simply do the following if path can never be null.

public List<String> getInfo( String path ) {
    DmgInfoGenerator info = new DmgInfoGenerator( this, path, parser );         
    return info.getInformation( );
}

[HermioneSW]

Meaning, whoever wrote the code does not seem to be confident that path can never be null but still left the door open for a potential NPE if path was null. Seems like a bug worth fixing...

[mumbel]

As you say, you're running static analysis and hoping others will validate your 'findings'. Where is the stack trace, whats the user interaction that causes this, what is the input to cause this, do you have binary to load or a script or maybe a java test class, anything?

[HermioneSW]

Whatever I have does not prevent you from answering the question so here I go again. If the person is so confident path can not be null why not simply do

public List<String> getInfo( String path ) {
    DmgInfoGenerator info = new DmgInfoGenerator( this, path, parser );         
    return info.getInformation( );
}

I'd be very happy to hear your lecture on why that is the case or anything else you have to say. By the way feel free to close the issue as I don't give a damn how great this codebase is written.

[ghizard]

I'm sorry you feel that @mumbel should be answering your question. He is a Ghidra user, who just like you, has decided to try to contribute to the project. @mumbel has developed language modules and has submitted them via PRs; that is why he shows up as a Contributor. We are thankful for people like @mumbel for helping to answer questions and thankful for anyone who is trying to help.

I cannot say for sure why ( path != null ) is shown on line 257 of DmgFileReader.java, but it stands to reason that it could be used (if not now, then in the future) by some other method than the one in DmgServer.java so it should remain there. However, to answer your question, it would be best to track back to whether path can be null in DmgServer.java before the call. The coder might have determined that it will not be a null value being passed into getInfo(), which (ignoring whether info.getInformation() can return null) would guarantee that no NPE would occur in DmgServer.java line 107.

The path variable comes from String path = parseLine(line) above the call to getInfo(). Looking at parseLine(), it stands to reason that path would not be null at the return because the code would have NPEd on the trim() method. I could keep tracking back further, but this just moves your question to another location. I think your analysis should keep tracking these back further. If you check subString(), it seems that it will not return null, so even trim() should not NPE.

As mentioned earlier, I believe that getInfo should keep the the null check in case it is used in other places. And I believe an NPE will not occur on line 107 of DmgServer.java due to the reasons I mentioned above.