Message encode/decode library

Question

I write protocol messages encoder/decoder for my project. All messages have 2-bytes header (1 byte - message type, 1 byte message sub-type) and body. There are two types of messages:

1. Plain message - no encoding
2. Encoded message - AES encoding

I would like to get your feedback on:

1. Overall design and coding style
2. Proposals for performance (it is very important) and memory usage improvement.

Comment #1: the following code is working code. Comment #2: the "Unsafe" stream classes are classes that extend ByteArray streams. The aim is to get direct access to internal stream buffer (it is protected).

public class PacketBuilder {
    
    /**
     * Creates DatagramPacket. The packet data is encoded.
     * The cipher should be initialized with Cipher.ENCRYPT_MODE.
     * 
     * @param data the data to be sent
     * @param cipher the cipher
     * @param sendTo the destination address
     * 
     * @return DatagramPacket
     * 
     * @throws NullPointerException if any of arguments is null
     * @throws EncodingException if error occurred during encoding
     */
    public static DatagramPacket buildEncoded(byte[] data, Cipher cipher, SocketAddress sendTo) 
    throws NullPointerException, EncodingException {

        Objects.requireNonNull(data);
        Objects.requireNonNull(cipher);
        Objects.requireNonNull(sendTo);

        try {

            UnsafeByteArrayOutputStream bytes = new UnsafeByteArrayOutputStream();
            CipherOutputStream cipherOutput = new CipherOutputStream(bytes, cipher);

            cipherOutput.write(data);
            cipherOutput.flush();
            cipherOutput.close();

            DatagramPacket packet = new DatagramPacket(bytes.getBuffer(), bytes.getByteCount());
            packet.setSocketAddress(sendTo);
            return packet;


        } catch (IllegalStateException | IOException e) {
            throw new EncodingException(e);
        }
    }

    /**
     * Creates DatagramPacket. The packet data is encoded.
     * The cipher should be initialized with Cipher.ENCRYPT_MODE.
     * 
     * @param type the message type
     * @param subType the message sub-type
     * @param string the string to be sent
     * @param cipher the cipher
     * @param sendTo the destination address
     * 
     * @return DatagramPacket
     * 
     * @throws NullPointerException if any of arguments is null
     * @throws EncodingException if error occurred during encoding  
     */
    public static DatagramPacket buildEncoded(byte type, byte subType, String string, Cipher cipher, SocketAddress sendTo)
    throws NullPointerException, EncodingException {
    
        Objects.requireNonNull(string);
        Objects.requireNonNull(cipher);
        Objects.requireNonNull(sendTo);

        byte[] data = null;

        try {
            data = string.getBytes(StandardCharsets.UTF_8.name());
        } catch (UnsupportedEncodingException e1) {};
    
        try {
            
            UnsafeByteArrayOutputStream bytes = new UnsafeByteArrayOutputStream();
            CipherOutputStream cipherOutput = new CipherOutputStream(bytes, cipher);
            
            cipherOutput.write(type);
            cipherOutput.write(subType);
            cipherOutput.write(data);
            cipherOutput.flush();
            cipherOutput.close();
            
            DatagramPacket packet = new DatagramPacket(bytes.getBuffer(), bytes.getByteCount());
            packet.setSocketAddress(sendTo);
            return packet;

        } catch (IllegalStateException | IOException e) {
            throw new EncodingException(e);
        }
    }

    /**
     * Creates DatagramPacket. The packet data is not encoded
     *  
     * @param type the message type
     * @param subType the message sub-type
     * @param data the data to be sent
     * @param sendTo the destination address
     * 
     * @return DatagramPacket  
     * 
     * @throws NullPointerException if any of arguments is null
     */
    public static DatagramPacket buildPlain(byte type, byte subType, String string, SocketAddress sendTo) 
    throws NullPointerException {

        Objects.requireNonNull(string);
        Objects.requireNonNull(sendTo);
        
        byte[] data = null;
        
        try {
            data = string.getBytes(StandardCharsets.UTF_8.name());
        } catch (UnsupportedEncodingException e1) {};

        
        UnsafeByteArrayOutputStream out = new UnsafeByteArrayOutputStream();
        
        try {
            out.write(type);
            out.write(subType);
            out.write(data);
            out.flush();
            out.close();
        } catch (IOException e) {}; //we cannot be here 

        DatagramPacket packet = new DatagramPacket(out.getBuffer(), out.getByteCount());
        packet.setSocketAddress(sendTo);
        return packet;
    }
    

public class PacketParser {
    
    /**
     * Decode packet data and creates message.  
     * The cipher have to be initialized with Cipher.DECRYPT_MODE
     * 
     * @param packet the packet to convert to message
     * @param cipher the cipher
     * 
     * @return message
     * 
     * @throws NullPointerException if any of arguments is null
     * @throws IllegalMessageException if packet data cannot be converted to message
     * @throws DecodingException if error occurred during decoding
     */
    public static Message parseEncoded(DatagramPacket packet,  Cipher cipher) 
    throws NullPointerException, IllegalMessageException, DecodingException {
    
        
        validate(packet);
        Objects.requireNonNull(cipher);
            
        UnsafeByteArrayInputStream bytesInput;
        CipherInputStream cipherInput = null;
        UnsafeByteArrayOutputStream bytesOutput = null;

        try {

            bytesInput = new UnsafeByteArrayInputStream(packet.getData(), 0, packet.getLength());
            cipherInput = new CipherInputStream(bytesInput, cipher);

            bytesOutput = new UnsafeByteArrayOutputStream();

            for (int b = cipherInput.read(); b > -1; b = cipherInput.read())
                bytesOutput.write(b);
            
            return new Message(bytesOutput.getBuffer(), bytesOutput.getByteCount(),  packet.getSocketAddress());

        } catch (IllegalStateException | IOException e) {
            throw new DecodingException(e);

        } finally {
            try {
                if (cipherInput != null) cipherInput.close();
                if (bytesOutput != null) bytesOutput.close();
            } catch (Exception e) {};
        }   
    }
    
    
    /**
     * Creates message from the given packet 
     * 
     * @param packet the packet to convert to message
     * 
     * @return message
     * 
     * @throws NullPointerException if any of arguments is null
     * @throws IllegalMessageException if packet data cannot be converted to message
     */
    public static Message parsePlain(DatagramPacket packet) 
    throws NullPointerException, IllegalMessageException  {
        
        validate(packet);
        
        return new Message(packet.getData(), packet.getLength(),  packet.getSocketAddress());
        
    }
    
    /*
     * Validates the packet. Throws exception if 
     * message has no header or header is illegal
     */
    private static void validate(DatagramPacket packet) 
    throws NullPointerException,  IllegalMessageException {

        Objects.requireNonNull(packet);

        if (packet.getLength() < HEADER_LENGTH)
            throw new IllegalMessageException("Message too short");

        int type = packet.getData()[0];
        int subType = packet.getData()[1];

        switch (type) {

        case Constants.REGISTRATION:
            
            switch (subType) {
            case Constants.HELLO:
            case Constants.KEY_EXCHANGE:
            case Constants.CLIENT_IF:
            case Constants.ACCESS:
            case Constants.ERROR:
                break;
            default:
                throw new IllegalMessageException("Unknown message");
            }
            
            break;

        case Constants.HEART_BEAT:
        case Constants.CONN_REQUEST:
            break;
        default: 
            throw new IllegalMessageException("Unknown message");

        }
    }

    
    public static class Message {
        
        final private byte type;
        final private byte subType;
        
        final private byte[] data;
        final private int length;
        
        final private SocketAddress address;
            
        private Message (byte[] buffer, int length, SocketAddress address) {
            
            this.type = buffer[0];
            this.subType = buffer[1];
            this.address = address;
            
            byte[] data = new byte[length - HEADER_LENGTH];
            System.arraycopy(buffer, HEADER_LENGTH, data, 0, data.length);
            
            this.data = data;
            this.length = data.length;
        }


        public byte getType() {
            return type;
        }

        public byte getSubType() {
            return subType;
        }

        public byte[] getData() {
            return data;
        }

        public int getLength() {
            return length;
        }


        public SocketAddress getAddress() {
            return address;
        }
        
    }
}

Answer 1

public class PacketBuilder {

...

public class PacketParser {

This I actually like very much, a clear distinction between objects that encode and decode. Or build and parse if you like.

---

First lets start with naming / terminology. AES is a block cipher, a method for encryption and decryption. When we are talking about encoding we should be talking about e.g. hexadecimal encoding, base 64 encoding, binary encodings (like the one for your message) or character encoding (UTF-8).

Most AES modes of operation such as CBC are - by themselves - not secure when used for transport mode. CBC with PKCS#7 padding (i.e. "PKCS5Padding" for Java) does not even preserve confidentiality when plaintext or padding oracles apply.

---

public static DatagramPacket buildEncoded(byte[] data, Cipher cipher, SocketAddress sendTo) 
throws NullPointerException, EncodingException {

Never pass a Cipher instance. Instances of Cipher are stateful, so you get into trouble if they are used e.g. in parallel. If you have to keep anything, keep the key, not the cipher. Cipher instances should be created locally. This also means that you don't have to put in comments such as Cipher.ENCRYPT_MODE (which you should check if you write that down).

Try and avoid static methods, prefer class instances to do the work for you (e.g. with a key and address as fields.

---

UnsafeByteArrayOutputStream seems to be part of an older Lucene package. Don't use experimental packages if this is the case. Also, please use and understand "try-with-resources". On the point of general stream handling: close should automatically flush, so there is no need for the call to flush().

A ByteBuffer may have been a better choice for this kind of operation; a datagram usually has a maximum size after all.

---

data = string.getBytes(StandardCharsets.UTF_8.name());

The good thing about string.getBytes(StandardCharsets.UTF_8) is that you don't have to handle the stupid exception for when the charset is not available. By calling name() you've removed that advantage.

---

public static DatagramPacket buildEncoded(byte[] data, Cipher cipher, SocketAddress sendTo) 
throws NullPointerException, EncodingException {

Let's repeat some code? Having two methods doing the same thing is fine if it is advantages to the user of the library, but then you should call a private method that does the actual building of the datagram.

You've got three things here:

1. creating the binary message;
2. possibly encrypting the binary message;
3. creating the datagram for it.

I'd like to see those in a design as three methods without code duplication.

---

public static Message parseEncoded(DatagramPacket packet,  Cipher cipher) 
throws NullPointerException, IllegalMessageException, DecodingException {

No, if you have a message here then your builder should accept messages as well.

Furthermore, things like NullPointerException should normally not be part of the signature of a method.

Because of the difference between encryption / decryption, it may not be clear to a user when either of the other two exceptions are thrown.

---

validate(packet);

Ah, only here we find out that some messages are acceptable and others are not. This is completely hidden from the user. There should be specialized message types instead, and you should not be able to send invalid packets using the builder, except maybe for error handling.

---

CipherInputStream cipherInput = null;

RED FLAG! You should never have to assign null to anything, especially when using try-with-resources.

---

packet.getLength()

I'm pretty sure that arrays in Java already store the length and start their index at 0. So there is really no need for all that it seems.

---

for (int b = cipherInput.read(); b > -1; b = cipherInput.read())`
            bytesOutput.write(b);

You'd be right if you thought that reading entire buffers at once would be faster than a one-by-one for loop.

---

int type = packet.getData()[0];

Zero should be a constant as well, e.g. HEADER_TYPE_INDEX = 0.

---

this.length = data.length;

Never needed.

---

public byte[] getData() {
    return data;
}

You forgot to clone the bytes here, unless you want to name this UnsafeMessage of course.

Answer 2

Nice to see clean, well formatted and commented code.

The buildEncoded builds the DatagramPacket, sends it and returns the packet. If the responsibilities cannot be split to separate methods, the reason should be documented in the code. Now the accurate name for the method would be buildSendAndReturnEncoded :).

Have you identified the string-based buildEncoded and buildPlain methods as performance bottlenecks? If not, they should create the message data byte array from the string and send that data to buildEncoded(byte[], Cipher, SocketAddress) instead of duplicating the DatagramPacket initialization code. The reason why maintainability is sacrificed on the altar of performance should always be clearly documented in code.

Same with the plaintext methods. If they are not performance hotspots, they should be scrapped and the crypto-methods should be reused with NullCipher instead.

All the "common io util" libraries I know provide some form of closeQuietly(Closeable) method so you don't need to repeat the try-if-not-null-close-catch-ignore mantra.