Skip to content

Fix two error paths that failed on their own - #615

Open
JakeHuneau wants to merge 1 commit into
OpenAS2:masterfrom
JakeHuneau:fix/error-paths-that-fail
Open

JakeHuneau wants to merge 1 commit into
OpenAS2:masterfrom
JakeHuneau:fix/error-paths-that-fail

Conversation

@JakeHuneau

Copy link
Copy Markdown
Contributor

Two places whose job is to report a problem fail while doing it, so the original problem is lost.

handleArchive throws a NullPointerException instead of reporting

IOUtil.handleArchive moves a file into the error directory after its message has already failed:

File destFile = null;
try {
    File archiveDir = IOUtil.getDirectoryFile(archiveDirectory);   // can throw
    destFile = new File(archiveDir, file.getName());
    destFile = IOUtil.moveFile(file, destFile, false);
} catch (IOException ioe) {
    ... + " to directory " + destFile.getAbsolutePath());          // destFile is null here

getDirectoryFile calls createDirs, which throws IOException("Directory '...' cannot be created") when the directory cannot be created: a read only mount, a permissions problem, or a network share that is temporarily unavailable. At that point destFile has not been assigned, so building the message throws a NullPointerException.

Two consequences. The real cause is replaced by a bare NPE, in the handler that runs after something has already gone wrong, which is the worst place to lose a diagnostic. And an NPE is unchecked, so it travels past the four callers in the directory poller and the resender that catch OpenAS2Exception.

It now names the directory it was given when it never got as far as a destination.

Two commands discard the error they just built

ViewMessageCommand and GetDataForCharts:

if (mpl == null || mpl.isEmpty()) {
    CommandResult cmdRes = new CommandResult(CommandResult.TYPE_ERROR);
    cmdRes.getResults().add("No DB tracking module available.");
}                                                    // no return
DbTrackingModule db = (DbTrackingModule) mpl.get(0);

The result is built and dropped, execution continues, and mpl.get(0) runs on the empty list. getModulesSupportingAction always returns a list and never null, so the failure is an IndexOutOfBoundsException rather than the NPE the shape of the guard suggests. Anyone running messages view or requesting the charts on a deployment without the database tracking module gets a stack trace instead of "No DB tracking module available."

ListMessagesCommand and GetMdnPathCommand make the same check and already return, so this brings the other two into line with them.

Testing

184 tests pass, 4 new for the archive handler. Restoring the null dereference fails 3 of the 4.

The reproduction uses a regular file as a parent path component so that creating the archive directory fails on any platform and as any user including root, rather than relying on a read only directory, which a privileged user would ignore and quietly skip the test. There is a separate read only case as well, guarded by assumptions.

IOUtil.handleArchive moves a file into the error directory after its message
has already failed. It built its failure message out of the destination file,
which is still null when it was working out the archive directory that failed.
That is exactly what happens when the error directory cannot be created, on a
read only or unavailable mount for instance, so the handler threw a
NullPointerException that replaced the real cause. Being unchecked it also
travelled past the callers that catch OpenAS2Exception. It now names the
directory it was given when it never got as far as a destination.

ViewMessageCommand and GetDataForCharts checked for having no message tracking
module, built the error result for it and then dropped it without returning, so
execution carried on and used the empty list anyway. Neither reported the
problem: both failed with an IndexOutOfBoundsException instead, since
getModulesSupportingAction always returns a list and never null. The two
sibling commands that make the same check, ListMessagesCommand and
GetMdnPathCommand, already return, so this brings the other two into line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant