@abh3 commented on this pull request.

In src/XrdTpc/XrdTpcStream.hh:

> @@ -80,7 +80,7 @@ private:
                 if (!size_desired) {return 0;}
             }
             int retval = stream.Write(m_offset, &m_buffer[0], size_desired, force);
-            if (retval < 0 && (static_cast<size_t>(retval) != size_desired)) {
+            if ((retval == -1) || (static_cast<size_t>(retval) != size_desired)) {

First, there is a merge conflict so this is now not mergeable. Specifically, after merging the Flush() fix, this created a conflict with this merge request surrounding the modified Flush(). The scale of the conflict is rather large and needs to be resolved by the author who knows best what was intended.

Second, I posted a my concerns that the patch may not be sufficient. In fact I alluded that there are two crash scenarios. The error handling on writes fix would appear the solve one of them and this is consistent across all sites and is rather frequent. The second, which is much less frequent involves pure heap corruption. From everything I can see it may very well be related to the concern I posted in ticket 1384. So, I will reiterate the essence here:

@bbockelm Code surrounding XrdTpcStream.cc:112 iteratively handles the buffers but the call in that line re-enters the same method (i.e. it is a recursive call) and invariably repeats the same code loop while the caller is still active in that loop. I don't necessarily see any protection of the caller's iterator and indeed, if one of the buffers is deleted the resumption of the iterative caller's block upon return may cause undefined results. Additionally, there is no practical limit on the recursive depth and strictly depends on how many buffers are queued; which may be substantial. Why was this done recursively in the first place, especially since it causes the same code block to repeat O(N**2). At best is seems the accounting will be wrong at the end at worst it may very well cause a heap corruption. Could you please address this issue?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/xrootd/xrootd/pull/1387#discussion_r563486302", "url": "https://github.com/xrootd/xrootd/pull/1387#discussion_r563486302", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

Use REPLY-ALL to reply to list

To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1