[ 
https://issues.apache.org/jira/browse/DISPATCH-1544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17016045#comment-17016045
 ] 

ASF GitHub Bot commented on DISPATCH-1544:
------------------------------------------

ganeshmurthy commented on issue #657: DISPATCH-1544: mask coverity false 
positive delivery use-after-free
URL: https://github.com/apache/qpid-dispatch/pull/657#issuecomment-574690566
 
 
   This commit only adds asserts ?
 
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> 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

Reply via email to