Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.
Comment: renamed based on Tim Halloran's suggestion

Applying a lock over a call to Synchronizing a method performing network transactions or declaring such a method synchronized can be problematic. Depending on the speed and reliability of the connection, synchronization can stall the program indefinitely causing a huge performance hit. At other times, it can result in temporary or permanent deadlock.

Noncompliant Code Example

This noncompliant code example involves the method sendPage() that sends a Page object containing information being passed between a client and a server. The method is synchronized to protect access to the array pageBuff. Calling writeObject() within the synchronized sendPage can result in a deadlock condition in high latency networks or when network connections are inherently lossy.

Code Block
bgColor#FFcccc
// Class Page is defined separately. It stores and returns the Page name via getName()

public final boolean SUCCESS = true;
public final boolean FAILURE = false;
Page[] pageBuff = new Page[MAX_PAGE_SIZE];

public synchronized boolean sendPage(Socket socket, String pageName) throws IOException {
  // Get the output stream to write the Page to
  ObjectOutputStream out = new ObjectOutputStream(socket.getOutputStream());

  Page targetPage = null;

  // Find the Page requested by the client (this operation requires synchronization)
  for(Page p : pageBuff) {
    if(p.getName().compareTo(pageName) == 0) {
      targetPage = p;
    }
  }

  // Page requested does not exist
  if(targetPage == null) {
    return FAILURE;
  } 

  // Send the Page to the client (does not require any synchronization)
  out.writeObject(targetPage);

  out.flush();
  out.close();
  return SUCCESS;
}

Compliant Solution

This compliant solution entails separating the actions into a sequence of steps:

...

Code Block
bgColor#ccccff
public boolean sendReply(Socket socket, String pageName) { // No synchronization
  Page targetPage = getPage(pageName); 

  if(targetPage == null)
    return FAILURE;

  return sendPage(socket, targetPage);
}

private synchronized Page getPage(String pageName) { // Requires synchronization
  Page targetPage = null;

  for(Page p : pageBuff) {
    if(p.getName().equals(pageName)) {
      targetPage = p;
    }
  }
  return targetPage;
}

public boolean sendPage(Socket socket, Page page){
  try{
    // Get the output stream to write the Page to
    ObjectOutputStream out = new ObjectOutputStream(socket.getOutputStream());

    // Send the Page to the client
    out.writeObject(page);

  } catch(IOException io){
    // If recovery is not possible return FAILURE
    return FAILURE;    
  } finally {
    out.flush();
    out.close();
  }  
  return SUCCESS;
}

Risk Assessment

If synchronized methods and statements contain network transactional logic, temporary or permanent deadlocks may result.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

CON20- J

low

probable

high

P2

L3

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

References

Wiki Markup
\[[Grosso 01|AA. Java References#Grosso 01]\] [Chapter 10: Serialization|http://oreilly.com/catalog/javarmi/chapter/ch10.html]
\[[JLS 05|AA. Java References#JLS 05]\] [Chapter 17, Threads and Locks|http://java.sun.com/docs/books/jls/third_edition/html/memory.html]
\[[Rotem 08|AA. Java References#Rotem 08]\] [Falacies of Distributed Computing Explained|http://www.rgoarchitects.com/Files/fallacies.pdf]

...