Skip to content

[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 from request.storedFilename, when in fact request was providing us request.filename.
  • as a result outSdMessage.filename was undefined, so when we passed it to File attachmentFile = new File(attachment.filename); in signald.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 either storedFilename (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 cause new InputFileStream to throw a FileNotFoundException, 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?

cc @mari @fdbk

Edited by aguestuser

Merge request reports