DO NOT REPLY [Bug 14578] New: - potential race condition

[bu...@apache.org]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14578>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14578

potential race condition

           Summary: potential race condition
           Product: XalanJ2
           Version: CurrentCVS
          Platform: Other
        OS/Version: Other
            Status: NEW
          Severity: Normal
          Priority: Other
         Component: org.apache.xalan.processor
        AssignedTo: xalan-dev@xml.apache.org
        ReportedBy: simon@ecnetwork.co.nz


The following code appears unsafe in threaded environments:

org.apache.xalan.Processor.StylesheetHandler, method init, line 1721:

    // Not sure about double-check of this flag, but
    // it seems safe...
    if (false == m_xpathFunctionsInited)
    {
      synchronized (this)
      {
        if (false == m_xpathFunctionsInited)
        {
          m_xpathFunctionsInited = true;

          Function func = new org.apache.xalan.templates.FuncDocument();

          FunctionTable.installFunction("document", func);

          // func = new org.apache.xalan.templates.FuncKey();
          // FunctionTable.installFunction("key", func);
          func = new org.apache.xalan.templates.FuncFormatNumb();

          FunctionTable.installFunction("format-number", func);
        }
      }
    }

The "init" method is an instance method. Calling synchronized(this) locks the
instance. That is not sufficient to protect access to static (class) member
m_xpathFunctionsInited.

Attached is a candidate patch. It acquires a lock on the class object rather
than the instance, and in addition *always* acquires the lock rather than
implementing the check-and-lock approach in the existing code, because:
* it is safer, easier to understand, obviously right (someone other than myself
had reservations about the check-and-lock code as shown by the existing
comments), and
* the very small performance benefit of avoiding locking is not significant here
as this is not a frequently-called method.