[DISPATCH-1544] Coverity false positive use-after-free error

• Type: Improvement
• Status: Closed
• Priority: Trivial
• Resolution: Fixed
• Affects Version/s: 1.11.0
• Fix Version/s: 1.11.0
• Assignee: Ken Giusti
• Reporter: Ken Giusti
• Created: 08/Jan/20 15:16
• Updated: 19/Mar/20 17:58
• Resolved: 15/Jan/20 16:14

Description

Coverity static analysis has started reporting use-after-free errors in the delivery state update logic.  I've reviewed these errors and I believe these are false positives.   I suspect that Coverity doesn't have enough context to understand the delivery refcount management done by this code to prevent use-after-free of the reported deliveries.

I've created a patch that adds assert checking of the refcounts to enforce the ownership state expected by the code.  This appears to "fix" the coverity issue in so much as when a CMAKE_BUILD_TYPE=Debug build is submitted for analysis (the asserts are present in Debug) the errors are resolved.   Non-Debug builds still report the error however so I'm also going update the patch to include annotations to ignore the errors.

I'm opening this Jira to solicit additional eyes on the problem in case I'm missing something and these errors are legit.

 

-----------------------------------------------------------------

4 new defect(s) introduced to Apache Qpid dispatch-router found with Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 4 of 4 defect(s)

• • CID 353022: (USE_AFTER_FREE)
    /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 729 in qdr_delivery_anycast_update_CT()

________________________________________________________________________________________________________

• • • CID 353022: (USE_AFTER_FREE)
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 729 in qdr_delivery_anycast_update_CT()
      723 assert(sys_atomic_get(&dlv->ref_count) > 1);
      724 assert(sys_atomic_get(&peer->ref_count) > 1);
      725 qdr_delivery_unlink_peers_CT(core, dlv, peer);
      726 }
      727
      728 if (dlink)
      >>> CID 353022: (USE_AFTER_FREE)
      >>> Calling "qdr_delivery_settled_CT" dereferences freed pointer "dlv".
      729 dlv_moved = qdr_delivery_settled_CT(core, dlv);
      730 }
      731
      732 //
      733 // If the delivery's link has a core endpoint, notify the endpoint of the update
      734 //
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 729 in qdr_delivery_anycast_update_CT()
      723 assert(sys_atomic_get(&dlv->ref_count) > 1);
      724 assert(sys_atomic_get(&peer->ref_count) > 1);
      725 qdr_delivery_unlink_peers_CT(core, dlv, peer);
      726 }
      727
      728 if (dlink)
      >>> CID 353022: (USE_AFTER_FREE)
      >>> Passing freed pointer "dlv" as an argument to "qdr_delivery_settled_CT".
      729 dlv_moved = qdr_delivery_settled_CT(core, dlv);
      730 }
      731
      732 //
      733 // If the delivery's link has a core endpoint, notify the endpoint of the update
      734 //

• • CID 353021: (USE_AFTER_FREE)
    /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 749 in qdr_delivery_anycast_update_CT()
    /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 739 in qdr_delivery_anycast_update_CT()

________________________________________________________________________________________________________

• • • CID 353021: (USE_AFTER_FREE)
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 749 in qdr_delivery_anycast_update_CT()
      743 //
      744 if (dlv_moved)
      745 qdr_delivery_decref_CT(core, dlv, "qdr_delivery_anycast_update CT - dlv removed from unsettled");
      746 if (peer_moved)
      747 qdr_delivery_decref_CT(core, peer, "qdr_delivery_anycast_update_CT - peer removed from unsettled");
      748 if (peer)
      >>> CID 353021: (USE_AFTER_FREE)
      >>> Passing freed pointer "peer" as an argument to "qdr_delivery_decref_CT".
      749 qdr_delivery_decref_CT(core, peer, "qdr_delivery_anycast_update_CT - allow free of peer");
      750
      751 return error_assigned;
      752 }
      753
      754
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 739 in qdr_delivery_anycast_update_CT()
      733 // If the delivery's link has a core endpoint, notify the endpoint of the update
      734 //
      735 if (dlink && dlink->core_endpoint)
      736 qdrc_endpoint_do_update_CT(core, dlink->core_endpoint, dlv, settled);
      737
      738 if (push || peer_moved)
      >>> CID 353021: (USE_AFTER_FREE)
      >>> Passing freed pointer "peer" as an argument to "qdr_delivery_push_CT".
      739 qdr_delivery_push_CT(core, peer);
      740
      741 //
      742 // Release the unsettled references if the deliveries were moved/settled
      743 //
      744 if (dlv_moved)
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 749 in qdr_delivery_anycast_update_CT()
      743 //
      744 if (dlv_moved)
      745 qdr_delivery_decref_CT(core, dlv, "qdr_delivery_anycast_update CT - dlv removed from unsettled");
      746 if (peer_moved)
      747 qdr_delivery_decref_CT(core, peer, "qdr_delivery_anycast_update_CT - peer removed from unsettled");
      748 if (peer)
      >>> CID 353021: (USE_AFTER_FREE)
      >>> Calling "qdr_delivery_decref_CT" dereferences freed pointer "peer".
      749 qdr_delivery_decref_CT(core, peer, "qdr_delivery_anycast_update_CT - allow free of peer");
      750
      751 return error_assigned;
      752 }
      753
      754
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 739 in qdr_delivery_anycast_update_CT()
      733 // If the delivery's link has a core endpoint, notify the endpoint of the update
      734 //
      735 if (dlink && dlink->core_endpoint)
      736 qdrc_endpoint_do_update_CT(core, dlink->core_endpoint, dlv, settled);
      737
      738 if (push || peer_moved)
      >>> CID 353021: (USE_AFTER_FREE)
      >>> Calling "qdr_delivery_push_CT" dereferences freed pointer "peer".
      739 qdr_delivery_push_CT(core, peer);
      740
      741 //
      742 // Release the unsettled references if the deliveries were moved/settled
      743 //
      744 if (dlv_moved)

• • CID 353020: (USE_AFTER_FREE)
    /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 1005 in qdr_delivery_mcast_outbound_update_CT()

________________________________________________________________________________________________________

• • • CID 353020: (USE_AFTER_FREE)
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 1005 in qdr_delivery_mcast_outbound_update_CT()
      999 // Note: qdr_delivery_mcast_outbound_settled_CT may free out_dlv!
      1000 if (settled && qdr_delivery_mcast_outbound_settled_CT(core, in_dlv, out_dlv, &dlv_moved)) { 1001 push_dlv = true; 1002 }
      1003
      1004 if (push_dlv || dlv_moved) { >>> CID 353020: (USE_AFTER_FREE) >>> Passing freed pointer "in_dlv" as an argument to "qdr_delivery_push_CT". 1005 qdr_delivery_push_CT(core, in_dlv); 1006 }
      1007 if (dlv_moved) { 1008 qdr_delivery_decref_CT(core, in_dlv, "qdr_delivery_mcast_outbound_update_CT - removed mcast peer from unsettled"); 1009 }
      1010 qdr_delivery_decref_CT(core, in_dlv, "qdr_delivery_mcast_outbound_update_CT - allow mcast peer free");
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 1005 in qdr_delivery_mcast_outbound_update_CT()
      999 // Note: qdr_delivery_mcast_outbound_settled_CT may free out_dlv!
      1000 if (settled && qdr_delivery_mcast_outbound_settled_CT(core, in_dlv, out_dlv, &dlv_moved)) {1001 push_dlv = true;1002 }
      1003
      1004 if (push_dlv || dlv_moved) { >>> CID 353020: (USE_AFTER_FREE) >>> Calling "qdr_delivery_push_CT" dereferences freed pointer "in_dlv". 1005 qdr_delivery_push_CT(core, in_dlv); 1006 }

      1007 if (dlv_moved)

      { 1008 qdr_delivery_decref_CT(core, in_dlv, "qdr_delivery_mcast_outbound_update_CT - removed mcast peer from unsettled"); 1009 }

      1010 qdr_delivery_decref_CT(core, in_dlv, "qdr_delivery_mcast_outbound_update_CT - allow mcast peer free");

• • CID 353019: Memory - illegal accesses (USE_AFTER_FREE)

________________________________________________________________________________________________________

• • • CID 353019: Memory - illegal accesses (USE_AFTER_FREE)
      /home/kgiusti/work/dispatch/qpid-dispatch/src/router_core/delivery.c: 826 in qdr_delivery_mcast_inbound_update_CT()
      820 if (unlink) { 821 // expect: in_dlv should not be freed here as caller must hold reference: 822 assert(sys_atomic_get(&in_dlv->ref_count) > 1); 823 qdr_delivery_unlink_peers_CT(core, in_dlv, out_peer); // may free out_peer! 824 }
      825
      >>> CID 353019: Memory - illegal accesses (USE_AFTER_FREE)
      >>> Calling "qdr_delivery_next_peer_CT" dereferences freed pointer "in_dlv".
      826 out_peer = qdr_delivery_next_peer_CT(in_dlv);
      827 }
      828
      829 if (settled) {
      830 assert(qdr_delivery_peer_count_CT(in_dlv) == 0);
      831 in_dlv->settled = true;

________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRZSbhom32dlDl11LWEm9nX1-2FDm2ydKRp2jKIMEChnF9qYjWDV40qhnoFf9KqJJs5gJkRt3r-2Bll2jeD6T5JeFcgC_GXVbCiBVihoVsavRYoBomC138NwPo21bY7ULJx-2Blbc6-2Bm7jghPWf9shMv58rQnTBWmoqpA4yGkw7Irka8-2F1mapS-2BlqITfMO7qBYycZX0CRsDtVVUgPaJZVLqhO3lDGP1Z3a7gDo-2F83Lwh4ic672c8spVZ1Z02NbeFL0d1BNBO8Nz7mQcucmnfcHUmlPy5YeUCE-2FKg3bEl0e3-2FIlMhTxoxCArT8Ei5dLOZ2dyqMlCZjo-3D

Issue Links

• links to: GitHub Pull Request #657