Skip to content

ODBC Mars issues #690

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

Open
mmcnabb-vms wants to merge 10 commits into
base: master
Choose a base branch
from
+139 −25
Open

ODBC Mars issues #690

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e782a89
ODBC MARS: Fix sliding window calculation
mmcnabb-vms Dec 23, 2025
675c58e
ODBC MARS: Extra debug logging
mmcnabb-vms Dec 23, 2025
138584b
ODBC MARS: Don't need to free MARS socket until statement free
mmcnabb-vms Dec 24, 2025
3ba42b5
ODBC Tests: option to suppress display of every command
mmcnabb-vms Dec 24, 2025
c309dfc
ODBC MARS: Expand the MARS1 test
mmcnabb-vms Dec 24, 2025
148c56b
ODBC MARS: Fix sliding window calculation (updated)
mmcnabb-vms Dec 29, 2025
533f6b4
ODBC MARS: Don't need to free MARS socket until statement free (updated)
mmcnabb-vms Dec 29, 2025
149edc9
Remove accidental debugging
mmcnabb-vms Dec 30, 2025
8f8e509
ODBC MARS test: Heap usage logging
mmcnabb-vms Dec 29, 2025
a18a448
Move declarations to top of function , for old compiler appveyor
mmcnabb-vms Jan 16, 2026
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
26 changes: 18 additions & 8 deletions src/odbc/odbc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,7 +872,13 @@ odbc_lock_statement(TDS_STMT* stmt)

/* try with MARS */
if (!tds)
{
tds = tds_alloc_additional_socket(dbc_tds->conn);
if (tds)
{
tdsdump_log(TDS_DBG_INFO1, "MARS SID %d allocated new TDSSOCKET\n", tds->sid);
Copy link
Contributor

@freddy77 freddy77 Dec 24, 2025

Choose a reason for hiding this comment

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

Maybe MARS SID %d allocated for new TDSSOCKET\n ?

Copy link
Contributor Author

@mmcnabb-vms mmcnabb-vms Dec 29, 2025

Choose a reason for hiding this comment

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

well, both of them are allocated really (the SID and the TDSSOCKET); how about with instead of for

Copy link
Contributor

@freddy77 freddy77 Jan 20, 2026

Choose a reason for hiding this comment

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

It sounds good to me

}
}
}
if (tds) {
tds->query_timeout = (stmt->attr.query_timeout != DEFAULT_QUERY_TIMEOUT) ?
Expand Down Expand Up @@ -922,15 +928,8 @@ odbc_unlock_statement(TDS_STMT* stmt)
tds_set_parent(tds, stmt->dbc);
stmt->tds = NULL;
}
#if ENABLE_ODBC_MARS
} else if (tds) {
if (tds->state == TDS_IDLE || tds->state == TDS_DEAD) {
assert(tds != stmt->dbc->tds_socket);
tds_free_socket(tds);
stmt->tds = NULL;
}
#endif
}
/* NOTE: MARS socket now released when statement freed. */
tds_mutex_unlock(&stmt->dbc->mtx);
}

Expand Down Expand Up @@ -4464,6 +4463,17 @@ odbc_SQLFreeStmt(SQLHSTMT hstmt, SQLUSMALLINT fOption, int force)
tds_free_param_results(stmt->params);
odbc_errs_reset(&stmt->errs);
odbc_unlock_statement(stmt);
#if ENABLE_ODBC_MARS
if ( stmt->tds && stmt->tds != stmt->dbc->tds_socket )
{
tds_free_socket(tds);
stmt->tds = NULL;
tdsdump_log(TDS_DBG_INFO1, "MARS SID %d socket freed\n", tds->sid);
if (!(tds->state == TDS_IDLE || tds->state == TDS_DEAD)) {
tdsdump_log(TDS_DBG_WARN, "MARS SID %d was not idle/dead\n", tds->sid);
}
Copy link
Contributor

@freddy77 freddy77 Dec 24, 2025

Choose a reason for hiding this comment

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

These looks like an use after free.

Copy link
Contributor Author

@mmcnabb-vms mmcnabb-vms Dec 29, 2025

Choose a reason for hiding this comment

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

Yes you're right; will have to reorder those statements (and re-word the messages, or remember the variables being logged)

}
#endif
tds_dstr_free(&stmt->cursor_name);
tds_dstr_free(&stmt->attr.qn_msgtext);
tds_dstr_free(&stmt->attr.qn_options);
Expand Down
15 changes: 12 additions & 3 deletions src/tds/packet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,7 @@ int
tds_read_packet(TDSSOCKET * tds)
{
#if ENABLE_ODBC_MARS
TDS_UINT this_seq;
TDSCONNECTION *conn = tds->conn;

tds_mutex_lock(&conn->list_mtx);
Expand Down Expand Up @@ -554,13 +555,21 @@ tds_read_packet(TDSSOCKET * tds)
tds->in_pos = 8;
tds->in_flag = tds->in_buf[0];

/* send acknowledge if needed */
if ((int32_t) (tds->recv_seq + 2 - tds->recv_wnd) >= 0)
tds_update_recv_wnd(tds, tds->recv_seq + 4);
/* Look ahead by up to 4 packets */
memcpy(&this_seq, ((char const *)packet->buf) + offsetof(TDS72_SMP_HEADER, seq), sizeof this_seq);
Copy link
Contributor

@freddy77 freddy77 Dec 24, 2025

Choose a reason for hiding this comment

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

I would use

this_seq = TDS_GET_A4LE(&((const TDS72_SMP_HEADER *) packet->buf)->seq);

also, consider that other code will still use tds->recv_seq, perhaps it would be better to update that in a different way.

Copy link
Contributor Author

@mmcnabb-vms mmcnabb-vms Dec 29, 2025

Choose a reason for hiding this comment

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

Agree about using TDS_GET_A4LE ; but is packet guaranteed to be correctly aligned for that cast?

The code in tds_connection_put_packet makes the same cast, so I guess it's not a problem. The code in tds_packet_read() uses memcpy(&mars_header, packet->buf, sizeof(mars_header));.

Copy link
Contributor

@freddy77 freddy77 Jan 24, 2026

Choose a reason for hiding this comment

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

I though I replied to this. Packet is an allocated buffer so it's surely aligned to 32 bit.

if (this_seq + 2 >= tds->recv_wnd)
Copy link
Contributor

@freddy77 freddy77 Dec 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This formulae is wrong if you have wrapping, the old one was correct.

Copy link
Contributor Author

@mmcnabb-vms mmcnabb-vms Dec 29, 2025

Choose a reason for hiding this comment

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

OK, you're right about the wrapping issue. I didn't like the old one as the C Standard doesn't define the behaviour of converting out-of-range unsigned values to signed integer, but FreeTDS is unlikely to be used on any system that doesn't behave as expected here.

Perhaps we could improve readability by borrowing TCP sequence macros, e.g. from BSD but with TDS_INT as the cast type (it should be the signed counterpart of TDS_UINT).

tds_update_recv_wnd(tds, this_seq + 4);

return tds->in_len;
}

#if ENABLE_EXTRA_CHECKS
{
TDS_UINT np = 0;
for (p_packet = &conn->packets; *p_packet; p_packet = &(*p_packet)->next) ++np;
tdsdump_log(TDS_DBG_NETWORK, "MARS SID %d queued packets %u\n", tds->sid, np);
}
#endif
/* network ok ? process network */
if (!conn->in_net_tds) {
tds_connection_network(conn, tds, 0);
Expand Down