Qpid
  1. Qpid
  2. QPID-3199

Severe but difficult to diagnose lock error in qpid::sys::StateMonitor

    Details

      Description

      The qpid::sys::StateMonitor class has 4 member functions liker this:
      void waitFor(Enum s)

      { ScopedWait(*this); while (s != state) wait(); }

      However the ScopeWait(*this); is in error, because it creates a temporary ScopedWait and then immediately destroys it. Instead of locking this and then unlocking it at the end of the function execution.

      What was meant was:
      void waitFor(Enum s)

      { ScopedWait w(*this); while (s != state) wait(); }

      which creates a local variable w which is destroyed at the end of the function execution unlocking this.

      It is possible that the compiler/options we use do not actually exhibit this bug. As the correct behaviour may not actually be implemented by all compilers.

      If this bug exhibits it might show as StateMonitor::waitFor() never detecting the desired state for instance. It is remarkable that this code has been in the codebase for 3.5 years and we've not noticed any bug caused by it.

      This bug was detected by the work on QPID-3004: The clang++ compiler detected that the value being returned by the constructor was not being used at all.

        Activity

        Hide
        Justin Ross added a comment -

        Approved for 0.10. Reviewed by Gordon Sim and Alan Conway.

        Show
        Justin Ross added a comment - Approved for 0.10. Reviewed by Gordon Sim and Alan Conway.
        Hide
        Alan Conway added a comment -

        I agree with Gordon. There's no risk and the bug potentially could cause very difficult to debug problems.

        Show
        Alan Conway added a comment - I agree with Gordon. There's no risk and the bug potentially could cause very difficult to debug problems.
        Hide
        Gordon Sim added a comment -

        fwiw: I see no risk in including this in 0.10 providing there is still time to do so.

        Show
        Gordon Sim added a comment - fwiw: I see no risk in including this in 0.10 providing there is still time to do so.
        Hide
        Andrew Stitcher added a comment -

        Fixed on trunk

        Show
        Andrew Stitcher added a comment - Fixed on trunk
        Hide
        Andrew Stitcher added a comment -

        Using this test program I established that gcc 4.5 does in fact perform as per the spec and so this is an actual bug in our code (maybe not triggered).

        #include <iostream>
        #include <string>

        using std::cout;
        using std::string;

        class Scoped {
        string s;

        public:
        Scoped(const string& s0) :
        s(s0)

        { cout << "Scoped const" << s << "\n"; }

        ~Scoped ()

        { cout << "Scoped destr" << s << "\n"; }

        };

        int main()

        { Scoped("A"); cout << "after A\n"; Scoped s("B"); cout << "after B\n"; }

        with results:

        $ make CXXFLAGS=-O3 destructor-sequence
        g++ -O3 destructor-sequence.cpp -o destructor-sequence
        $ ./destructor-sequence
        Scoped constA
        Scoped destrA
        after A
        Scoped constB
        after B
        Scoped destrB
        $

        Show
        Andrew Stitcher added a comment - Using this test program I established that gcc 4.5 does in fact perform as per the spec and so this is an actual bug in our code (maybe not triggered). #include <iostream> #include <string> using std::cout; using std::string; class Scoped { string s; public: Scoped(const string& s0) : s(s0) { cout << "Scoped const" << s << "\n"; } ~Scoped () { cout << "Scoped destr" << s << "\n"; } }; int main() { Scoped("A"); cout << "after A\n"; Scoped s("B"); cout << "after B\n"; } with results: $ make CXXFLAGS=-O3 destructor-sequence g++ -O3 destructor-sequence.cpp -o destructor-sequence $ ./destructor-sequence Scoped constA Scoped destrA after A Scoped constB after B Scoped destrB $

          People

          • Assignee:
            Andrew Stitcher
            Reporter:
            Andrew Stitcher
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development