Why is my image coming out garbled?

Question

I've got some Java code using a servlet and Apache Commons FileUpload to upload a file to a set directory. It's working fine for character data (e.g. text files) but image files are coming out garbled. I can open them but the image doesn't look like it should. Here's my code:

Servlet

protected void doPost(HttpServletRequest request, HttpServletResponse response)
    throws ServletException, IOException {
    try {
      String customerPath = "\\leetest\\";

      // Check that we have a file upload request
      boolean isMultipart = ServletFileUpload.isMultipartContent(request);

      if (isMultipart) {
        // Create a new file upload handler
        ServletFileUpload upload = new ServletFileUpload();

        // Parse the request
        FileItemIterator iter = upload.getItemIterator(request);
        while (iter.hasNext()) {
          FileItemStream item = iter.next();
          String name = item.getFieldName();
          if (item.isFormField()) {
            // Form field.  Ignore for now
          } else {
            BufferedInputStream stream = new BufferedInputStream(item
                .openStream());
            if (stream == null) {
              LOGGER
                  .error("Something went wrong with fetching the stream for field "
                      + name);
            }

            byte[] bytes = StreamUtils.getBytes(stream);
            FileManager.createFile(customerPath, item.getName(), bytes);

            stream.close();
          }
        }
      }
    } catch (Exception e) {
      throw new UploadException("An error occured during upload: "
          + e.getMessage());
    }
}

StreamUtils.getBytes(stream) looks like:

public static byte[] getBytes(InputStream src, int buffsize)
      throws IOException {
    ByteArrayOutputStream byteStream = new ByteArrayOutputStream();
    byte[] buff = new byte[buffsize];
    while (true) {
      int nBytesRead = src.read(buff);
      if (nBytesRead < 0) {
        break;
      }
      byteStream.write(buff);
    }

    byte[] result = byteStream.toByteArray();
    byteStream.close();

    return result;
}

And finally FileManager.createFile looks like:

public static void createFile(String customerPath, String filename,
      byte[] fileData) throws IOException {
    customerPath = getFullPath(customerPath + filename);
    File newFile = new File(customerPath);
    if (!newFile.getParentFile().exists()) {
      newFile.getParentFile().mkdirs();
    }

    FileOutputStream outputStream = new FileOutputStream(newFile);
    outputStream.write(fileData);
    outputStream.close();
  }

Can anyone spot what I'm doing wrong?

Cheers, Lee

Answer 1

One thing I don't like is here in this block from StreamUtils.getBytes():

 1 while (true) {
 2   int nBytesRead = src.read(buff);
 3   if (nBytesRead < 0) {
 4     break;
 5   }
 6   byteStream.write(buff);
 7 }

At line 6, it writes the entire buffer, no matter how many bytes are read in. I am not convinced this will always be the case. It would be more correct like this:

 1 while (true) {
 2   int nBytesRead = src.read(buff);
 3   if (nBytesRead < 0) {
 4     break;
 5   } else {
 6     byteStream.write(buff, 0, nBytesRead);
 7   }
 8 }

Note the 'else' on line 5, along with the two additional parameters (array index start position and length to copy) on line 6.

I could imagine that for larger files, like images, the buffer returns before it is filled (maybe it is waiting for more). That means you'd be unintentionally writing old data that was remaining in the tail end of the buffer. This is almost certainly happening most of the time at EoF, assuming a buffer > 1 byte, but extra data at EoF is probably not the cause of your corruption...it is just not desirable.