read.c (tds_get_n): Avoid potential hangs on short replies.#585
Conversation
Don't bother trying to receive data following a packet marked as last; rather, bail if still short. Whenever bailing, close the socket on the way out to contain the damage from having fallen out of sync. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>
| } | ||
| need -= have; | ||
| if (TDS_UNLIKELY(tds_read_packet(tds) < 0)) | ||
| if (TDS_UNLIKELY(tds->recv_packet->capacity < 2 |
There was a problem hiding this comment.
capacity can't be so small
There was a problem hiding this comment.
Fair point; I just wanted to preclude any possibility that the following array access would be invalid.
| need -= have; | ||
| if (TDS_UNLIKELY(tds_read_packet(tds) < 0)) | ||
| if (TDS_UNLIKELY(tds->recv_packet->capacity < 2 | ||
| || tds->in_buf[1] != 0 |
There was a problem hiding this comment.
Maybe you want to test only the final bit and not all the flags. It makes sense, if no more packets are expected data should not continue on next packet. Wondering if tds_get_n would be called for first bytes of the packet series. Not sure if this can even happen however. I don't think cancellation apply here.
There was a problem hiding this comment.
Ah, yeah, testing just the low-order bit is more appropriate, though I'm not sure how much difference it makes in practice.
The situation originally motivating this change was canceling blob retrieval, which at least in the case of the OpenServer instance I was using, could yield a truncated row response giving a blob's full length but containing only part of its body, leading to loss of synchronization that would otherwise typically culminate in hangs. Perhaps the server should have acknowledged cancellation more clearly, but it did at least set that flag correctly.
There was a problem hiding this comment.
Interesting! So you are telling the way Open Server handles cancellation is different? Maybe we can support it.
There was a problem hiding this comment.
Yes, at least historically. Maybe it's been fixed since, but a little extra caution here can't hurt.
|
Merged, with minor changes. Sorry for the delay. One thing that puzzled me is why it did not fail when we get to a new packet; probably the packet always start with a byte read by |
2 participants
Don't bother trying to receive data following a packet marked as last; rather, bail if still short. Whenever bailing, close the socket on the way out to contain the damage from having fallen out of sync.
Split from #555.