[
https://issues.apache.org/jira/browse/DISPATCH-1544?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ken Giusti resolved DISPATCH-1544.
----------------------------------
Resolution: Fixed
> Coverity false positive use-after-free error
> --------------------------------------------
>
> Key: DISPATCH-1544
> URL: https://issues.apache.org/jira/browse/DISPATCH-1544
> Project: Qpid Dispatch
> Issue Type: Improvement
> Affects Versions: 1.11.0
> Reporter: Ken Giusti
> Assignee: Ken Giusti
> Priority: Trivial
> Fix For: 1.11.0
>
>
> 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]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
