3
\$\begingroup\$

I'm not sure what's the best way to deal with streams that need to be closed whenever I'm done using them? Usually the need to also catch the exception inside the finally statement is very, very annoying. Am I doing this right?

public class FileSender {

    public static final String KEY = "00000000000000000000000000000000";
    public static final String ALGORITHM = "AES";
    public static final String FILE = "C:\\Users\\Andres\\Desktop\\file.txt";
    public static final String HASH_ALG = "MD5";

    private String filePath;
    private String serverMachine;
    private int serverPort;

    public FileSender(String filePath, String serverMachine, int serverPort){
        this.filePath = filePath;
        this.serverMachine = serverMachine;
        this.serverPort = serverPort;
    }

    public void sendFile(){
        CipherOutputStream cipherOut = null;
        Socket client = null;
        String checkSum;
        try {
            System.out.println("Running client ...");
            client = connectToServer();
            checkSum = getHashChecksum();
            cipherOut = initCipherStream(client);
            transferData(cipherOut, checkSum);
        } catch (Exception e) {
            System.out.println(e.getMessage());
        } finally {

        }
    }

    private String getHashChecksum() {
        FileInputStream fis = null;
        try {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);
            fis = new FileInputStream(this.filePath);
            byte[] byteArray = new byte[1024];
            int bytesCount = 0;
            while ((bytesCount = fis.read(byteArray)) >= 0)
                md.update(byteArray, 0, bytesCount);
            byte[] hash = md.digest();
            fis.close();
            return String.format("%032x",new BigInteger(1, hash));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        } finally {
            try {
                fis.close();
            } catch (IOException e) {
                throw new RuntimeException("Error in checksum. Provided file can't be closed.");
            }
        }
    }

    private Socket connectToServer() throws Exception{
        try {
            Socket client = new Socket(this.serverMachine, this.serverPort);
            return client;
        } catch (IOException e) {
            throw new Exception("Server can't be reached");
        }
    }

    private CipherOutputStream initCipherStream(Socket client) {
        SecretKeySpec keySpec = new SecretKeySpec(hex2byte(KEY), ALGORITHM);
        Cipher cipher;
        try {
            cipher = Cipher.getInstance(ALGORITHM + "/ECB/PKCS5PADDING");
            cipher.init(Cipher.ENCRYPT_MODE, keySpec);
            return new CipherOutputStream(client.getOutputStream(), cipher);
        } catch (Exception  e) {
            e.printStackTrace();
            throw new RuntimeException("Error in cipher stream initialization.");
        }

    }

    private void transferData(CipherOutputStream cipherOut, String checksum) {
        FileInputStream fileReader = null;
        try {
            //Missing here: send MD5 checksum. TODO
            fileReader = new FileInputStream(this.filePath);
            byte[] fileBuffer = new byte[1024];
            int len;
            while ((len = fileReader.read(fileBuffer)) != -1)
                cipherOut.write(fileBuffer, 0, len);
        } catch (Exception e) {
            throw new RuntimeException("File " + this.filePath + " not found");
        } finally{
            try {
                if(fileReader != null) fileReader.close();
            } catch (IOException e) {
                throw new RuntimeException("Socket stream can't be closed");
            }
        }
    }

    private byte[] hex2byte(String s) {
        return DatatypeConverter.parseHexBinary(s);
    }

    public static void main(String [] args) throws Exception {
        FileSender fileSender = new FileSender(FILE, "localhost", 8081);
        fileSender.sendFile();
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You are probably interested in "try-with-resources" that was introduced in Java 7. \$\endgroup\$ – Simon Forsberg May 11 '17 at 17:00
  • \$\begingroup\$ Could you post the rendered code just to check the answer? I'm not sure how are the dynamics in CodeReview, so I don't know if asking for this is ok. Maybe just a piece of the code is what is usually given as answers? \$\endgroup\$ – AFP_555 May 11 '17 at 17:10
  • \$\begingroup\$ Also, what would you do in case a mathod depends on two resources, not just one? Let's say I have two files and I want to compare line by line if both files have the same text. I'd need to Streams to read the file. \$\endgroup\$ – AFP_555 May 11 '17 at 17:14
6
\$\begingroup\$

try-with-resources

        FileInputStream fis = null;
        try {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);
            fis = new FileInputStream(this.filePath);
            byte[] byteArray = new byte[1024];
            int bytesCount = 0;
            while ((bytesCount = fis.read(byteArray)) >= 0)
                md.update(byteArray, 0, bytesCount);
            byte[] hash = md.digest();
            fis.close();
            return String.format("%032x",new BigInteger(1, hash));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        } finally {
            try {
                fis.close();
            } catch (IOException e) {
                throw new RuntimeException("Error in checksum. Provided file can't be closed.");
            }
        }

In a normal run, you try to close the stream twice. You do not need to call close() before the return. The finally block will take care of it.

There is also an issue. If there is an exception in the getInstance call, you will attempt to dereference a null pointer in the finally block.

You can rewrite this

        try (FileInputStream fis = new FileInputStream(this.filePath)) {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);

            byte[] bytes = new byte[1024];
            for (int bytesCount = fis.read(bytes); bytesCount >= 0; bytesCount = fis.read(bytes)) {
                md.update(byteArray, 0, bytesCount);
            }

            return String.format("%032x", new BigInteger(1, md.digest()));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        }

This uses a try-with-resources so that you don't have to manually close the resource.

I prefer names like bytes to byteArray as being simpler and more focused on what the data means logically than how it is stored physically. You can read more about this in discussions of Hungarian notation. This is somewhat controversial. I mostly mention it because the code was scrolling when it was named the Hungarian notation way.

Using the for loop to replace the while reduces the scope of the bytesCount variable. Other than that, both do the the same thing.

I added some vertical whitespace to break things up.

It's not necessary to assign the result of md.digest() to a variable. We can pass it directly to the BigInteger constructor. Either way works though.

\$\endgroup\$
  • \$\begingroup\$ Very unusual way to use the for loop. I love it. \$\endgroup\$ – AFP_555 May 13 '17 at 2:34
3
\$\begingroup\$

mdfst13 already gave some useful comments regarding the message digest, so I'd like to add something regarding transferData:

When you copy from a file path to a stream, there's no need to write this manually anymore. Files to the rescue:

Files.copy(this.filePath, cipherOut);

Furthermore, check whether you can alter your transfer logic, so that you utilize the streams decorator pattern to the fullest. I imagine, that you could actually wrap the output stream into a digest ouput stream first and in a cyper output stream second, so that you need only a single read-file-and-copy-to-stream operation. (See https://docs.oracle.com/javase/8/docs/api/java/security/DigestOutputStream.html)

\$\endgroup\$
  • \$\begingroup\$ Amazing, thanks. But I have a question. I found that very big files have to be sent using a buffer. This way the whole file isn't sent all at once. This nice approach you showed will allow the use of a buffer? \$\endgroup\$ – AFP_555 May 13 '17 at 2:32
  • \$\begingroup\$ Yes, under the hood it uses a buffer of 8 kb. \$\endgroup\$ – mtj May 13 '17 at 4:12
  • \$\begingroup\$ And would this 8 kb buffer allow for AES 128 encryption? \$\endgroup\$ – AFP_555 May 14 '17 at 0:25
  • \$\begingroup\$ Provided that the cypher output stream works as designed, I don't see why not. \$\endgroup\$ – mtj May 14 '17 at 12:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.