Decoder memory allocation failure causes deadlock

[Niels Dekker]

When a memory allocation failure occurs during decoding, we sometimes get into a deadlock (*), during a later stage. For example, when we try to cleanup() a decoder registration. The deadlock is caused by the fact that the operator new might throw an exception (std::bad_alloc). If this happens just before a codecLock.unlock() call, the codecLock will typically remains locked. Another attempt to lock(), for example by cleanup() leads to a deadlock.

Proposed fix (3 steps):

1. Add a little helper class, that does unlock() during destruction.

Code: Select all

  /** Automatically unlocks the specified read/write lock when it goes out of scope.
  */
  class AutoUnlock
  {
    OFReadWriteLock& readWriteLock;

  public:
    /** explicit constructor */
    explicit AutoUnlock(OFReadWriteLock& arg) : readWriteLock(arg) 
    {
    }

    /** destructor */
    ~AutoUnlock()
    {
      readWriteLock.unlock();
    }

  private:

    /** unimplemented private copy constructor */
    AutoUnlock(const AutoUnlock& arg);

    /** unimplemented private assignment operator */
    AutoUnlock& operator=(const AutoUnlock& arg);
  };

2. Remove all (nine!) codecLock.unlock() calls from dccodec.cc.

3. Declare a local AutoUnlock object at the begin of each scope that is opened after if (0 == codecLock.XXlock()). Hereby XXlock() is either rdlock() or wrlock(), for example:

Code: Select all

  #ifdef _REENTRANT
    if (0 == codecLock.rdlock())
    {
      const AutoUnlock constAutoUnlock(codecLock);
  #endif

(Nine such AutoUnlock objects must be declared, in dccodec.cc.)

Hope you can take a look at this issue. If you want, I can send you a patched version of dccodec.cc.

Niels

(*) I'm not entirely sure if "deadlock" is the right term here, but certainly, it blocks the program.

[Niels Dekker]

Did anyone of you already have a look at the (potential) deadlock issue I reported on April 27?

I think the fix is rather simple, as it is limited to a single DCMTK source file. Would it be helpful if we mail our modified version of dcmdata/libsrc/dccodec.cc to DICOM (at) OFFIS?

Kind regards,

Niels

[Uli Schlachter]

Hi,

thanks for your report.
I went forward and implemented this. I added a new class OFReadWriteLocker to ofstd which implements rdlock(), wrlock(), tryrdlock(), trywrlock() and unlock(). Only if the read/write lock is currently locked through this new class when it is destructed, it will unlock the underlying lock. http://git.dcmtk.org/web?p=dcmtk.git;a= ... b2aa25c2f0
Using this I implemented your changes to dccodec.c. http://git.dcmtk.org/web?p=dcmtk.git;a= ... 60965b9543

Thank you for flying DCMTK and for reporting this issue. Please enjoy the rest of your flight.
Uli

[Niels Dekker]

Thanks Uli! I just had a look at your code, and I think it should work fine. Cool!

BTW I'm not flying, I'm going by train!