Skip to content

read.c (tds_get_n): Avoid potential hangs on short replies. #585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ucko wants to merge 1 commit into from
Closed

read.c (tds_get_n): Avoid potential hangs on short replies. #585

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/tds/read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,12 @@ tds_get_n(TDSSOCKET * tds, void *dest, size_t need)
dest = (char *) dest + have;
}
need -= have;
if (TDS_UNLIKELY(tds_read_packet(tds) < 0))
if (TDS_UNLIKELY(tds->recv_packet->capacity < 2
Copy link
Contributor

@freddy77 freddy77 Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capacity can't be so small

Copy link
Contributor Author

@ucko ucko Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point; I just wanted to preclude any possibility that the following array access would be invalid.

|| tds->in_buf[1] != 0
Copy link
Contributor

@freddy77 freddy77 Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ucko ucko Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@freddy77 freddy77 Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! So you are telling the way Open Server handles cancellation is different? Maybe we can support it.

Copy link
Contributor Author

@ucko ucko Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, at least historically. Maybe it's been fixed since, but a little extra caution here can't hurt.

|| tds_read_packet(tds) < 0)) {
tds_close_socket(tds); /* evidently out of sync */
return false;
}
}
if (need > 0) {
/* get the remainder if there is any */
Expand Down