[ni] defensively parse outbound attachments to prevent NPE on resend
SYMPTOM:
- when resending rate-limited messages, signald throws an NPE when
trying to construct a file handle from the
attachment.filename
field
this is the error message:
signald_1 | java.lang.NullPointerException: null
signald_1 | at java.io.File.<init>(File.java:277) ~[?:1.8.0_232]
signald_1 | at io.finn.signald.SocketHandler.send(SocketHandler.java:233) ~[signald.jar:unspecified]
signald_1 | at io.finn.signald.SocketHandler.handleRequest(SocketHandler.java:140) [signald.jar:unspecified]
signald_1 | at io.finn.signald.SocketHandler.run(SocketHandler.java:125) [signald.jar:unspecified]
signald_1 | at java.lang.Thread.run(Thread.java:748) [?:1.8.0_232]
RELEVANT CODE:
in signald:
// io.signald.SocketHandler.send(SocketHandler.java:233
List<SignalServiceAttachment> attachments = null;
if (request.attachments != null) {
attachments = new ArrayList<>(request.attachments.size());
for (JsonAttachment attachment : request.attachments) {
try {
File attachmentFile = new File(attachment.filename);
InputStream attachmentStream = new FileInputStream(attachmentFile);
final long attachmentSize = attachmentFile.length();
String mime = Files.probeContentType(attachmentFile.toPath());
if (mime == null) {
mime = "application/octet-stream";
}
attachments.add(new SignalServiceAttachmentStream(attachmentStream, mime, attachmentSize, Optional.of(attachmentFile.getName()), attachment.voiceNote, attachment.getPreview(), attachment.width, attachment.height, Optional.fromNullable(attachment.caption), Optional.fromNullable(attachment.blurhash), null));
} catch (IOException e) {
throw new AttachmentInvalidException(attachment.filename, e);
}
}
}
in decompiled java bytecode:
// java.io.File.class:61
public File(String var1) {
if (var1 == null) {
throw new NullPointerException();
} else {
this.path = fs.normalize(var1);
this.prefixLength = fs.prefixLength(this.path);
}
}
// java.io.FileInputStream.class:23
public FileInputStream(File var1) throws FileNotFoundException {
this.closeLock = new Object();
String var2 = var1 != null ? var1.getPath() : null;
SecurityManager var3 = System.getSecurityManager();
if (var3 != null) {
var3.checkRead(var2);
}
if (var2 == null) {
throw new NullPointerException();
} else if (var1.isInvalid()) {
throw new FileNotFoundException("Invalid file path");
} else {
this.fd = new FileDescriptor();
this.fd.attach(this);
this.path = var2;
this.open(var2);
this.altFinalizer = getFinalizer(this);
if (this.altFinalizer == null) {
FileCleanable.register(this.fd);
}
}
}
HYPOTHESIS:
- signald is throwing an NPE, because we tried to parse
outSdMessagefilename
fromrequest.storedFilename
, when in factrequest
was providing usrequest.filename
. - as a result
outSdMessage.filename
was undefined, so when we passed it toFile attachmentFile = new File(attachment.filename);
insignald.SocketHandler.send(SocketHandler.java:233)
,attachment.filename
is null and the constructore throws an NPE
ATTEMPTED FIX:
- defensively try to parse a
filename
field from eitherstoredFilename
(if present),filename
(if not), or provide an empty string - in the ideal case, this grabs the filename from whatever field it was provided in
- as a fallback, it at least prevents the original NPE by providing an
empty string, which will prevent
new File
from crashing, but causenew InputFileStream
to throw aFileNotFoundException
, which fortunately is okay (or at least better), because that exception is handled by signald and should give us slightly better reporting and ability to recover?
Edited by aguestuser