Skip to content

Commit 3ed053d

Browse files
committed
fix buffer underrun for wrong received buffer size (#199)
This commit fixes an underrun in the utility function copying a wide-char string to the client application. The impact is limited and it can only happen if the received size of the client buffer is of the wrong size (<=0). The commit also fixes a logging error happening if this function needs to truncate the output. The macro for a format specifier with no precision was provided instead of the needed one with precision. This could potentially crash the client application in case the string to be logged in not 0-terminated, logging is explicitely enabled at an INFO (or higher) level and truncation needs to be applied (cherry picked from commit ea2814b)
1 parent 6e645fd commit 3ed053d

File tree

3 files changed

+61
-11
lines changed

3 files changed

+61
-11
lines changed

driver/util.c

+10-8
Original file line numberDiff line numberDiff line change
@@ -524,7 +524,7 @@ size_t json_escape_overlapping(char *str, size_t inlen, size_t outlen)
524524
* wstr according to avaialble space and indicates the available bytes to copy
525525
* back into provided buffer (if not NULL).
526526
*/
527-
SQLRETURN write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
527+
SQLRETURN TEST_API write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
528528
SQLSMALLINT /*B*/avail, SQLSMALLINT /*B*/*usedp)
529529
{
530530
size_t wide_avail;
@@ -533,8 +533,8 @@ SQLRETURN write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
533533
"len; out-len @0x%p.", src->cnt, LWSTR(src), dest, avail, usedp);
534534

535535
/* cnt must not count the 0-term (XXX: ever need to copy 0s?) */
536-
assert(src->cnt <= 0 || src->str[src->cnt - 1]);
537-
assert(src->cnt <= 0 || src->str[src->cnt] == 0);
536+
assert(src->cnt <= 0 || src->str[src->cnt - 1] != L'\0');
537+
assert(src->cnt <= 0 || src->str[src->cnt] == L'\0');
538538

539539
if (usedp) {
540540
/* how many bytes are available to return (not how many would be
@@ -547,20 +547,22 @@ SQLRETURN write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
547547

548548
if (dest) {
549549
/* needs to be multiple of SQLWCHAR units (2 on Win) */
550-
if (avail % sizeof(SQLWCHAR)) {
551-
ERRH(hnd, "invalid buffer length provided: %d.", avail);
550+
if (avail < 0 || avail % sizeof(SQLWCHAR)) {
551+
ERRH(hnd, "invalid buffer length provided: %hd.", avail);
552552
RET_HDIAGS(hnd, SQL_STATE_HY090);
553553
} else {
554554
wide_avail = avail/sizeof(SQLWCHAR);
555555
}
556556

557557
/* '=' (in <=), since src->cnt doesn't count the \0 */
558558
if (wide_avail <= src->cnt) {
559-
wcsncpy(dest, src->str, wide_avail - /* 0-term */1);
560-
dest[wide_avail - 1] = 0;
559+
if (0 < wide_avail) {
560+
wcsncpy(dest, src->str, wide_avail - /* 0-term */1);
561+
dest[wide_avail - 1] = L'\0';
562+
}
561563

562564
INFOH(hnd, "not enough buffer size to write required string (plus "
563-
"terminator): `" LWPD "` [%zu]; available: %zu.",
565+
"terminator): `" LWPDL "` [%zu]; available: %zu.",
564566
LWSTR(src), src->cnt, wide_avail);
565567
RET_HDIAGS(hnd, SQL_STATE_01004);
566568
} else {

driver/util.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ size_t json_escape_overlapping(char *str, size_t inlen, size_t outlen);
270270
* wstr according to avaialble space and indicates the available bytes to copy
271271
* back into provided buffer (if not NULL).
272272
*/
273-
SQLRETURN write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
273+
SQLRETURN TEST_API write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
274274
SQLSMALLINT /*B*/avail, SQLSMALLINT /*B*/*usedp);
275275

276276
/*

test/test_util.cc

+50-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
extern "C" {
88
#include "util.h"
9+
#include "handles.h"
910
} // extern C
1011

1112
#include <gtest/gtest.h>
@@ -105,6 +106,55 @@ TEST_F(Util, wstr_to_utf8_no_nts) {
105106
free(dst.str);
106107
}
107108

109+
TEST_F(Util, write_wstr_invalid_avail) {
110+
wstr_st src = WSTR_INIT(SRC_STR);
111+
SQLWCHAR dst[sizeof(SRC_STR)];
112+
esodbc_env_st env = {0};
113+
SQLSMALLINT used;
114+
115+
SQLRETURN ret = write_wstr(&env, dst, &src, sizeof(*dst) + 1, &used);
116+
ASSERT_FALSE(SQL_SUCCEEDED(ret));
117+
}
118+
119+
TEST_F(Util, write_wstr_0_avail) {
120+
wstr_st src = WSTR_INIT(SRC_STR);
121+
SQLWCHAR dst[sizeof(SRC_STR)];
122+
esodbc_env_st env = {0};
123+
SQLSMALLINT used;
124+
125+
SQLRETURN ret = write_wstr(&env, dst, &src, /*avail*/0, &used);
126+
assert(SQL_SUCCEEDED(ret));
127+
ASSERT_EQ(used, src.cnt * sizeof(*dst));
128+
}
129+
130+
TEST_F(Util, write_wstr_trunc) {
131+
wstr_st src = WSTR_INIT(SRC_STR);
132+
SQLWCHAR dst[sizeof(SRC_STR)];
133+
esodbc_env_st env = {0};
134+
SQLSMALLINT used;
135+
136+
SQLRETURN ret = write_wstr(&env, dst, &src,
137+
(SQLSMALLINT)((src.cnt - 1) * sizeof(*dst)), &used);
138+
assert(SQL_SUCCEEDED(ret));
139+
ASSERT_EQ(used, src.cnt * sizeof(*dst));
140+
ASSERT_EQ(dst[src.cnt - 2], L'\0');
141+
ASSERT_EQ(wcsncmp(src.str, dst, src.cnt - 2), 0);
142+
}
143+
144+
TEST_F(Util, write_wstr_copy) {
145+
wstr_st src = WSTR_INIT(SRC_STR);
146+
SQLWCHAR dst[sizeof(SRC_STR)];
147+
esodbc_env_st env = {0};
148+
SQLSMALLINT used;
149+
150+
SQLRETURN ret = write_wstr(&env, dst, &src, (SQLSMALLINT)sizeof(dst),
151+
&used);
152+
assert(SQL_SUCCEEDED(ret));
153+
ASSERT_EQ(used, src.cnt * sizeof(*dst));
154+
ASSERT_EQ(dst[src.cnt], L'\0');
155+
ASSERT_EQ(wcscmp(src.str, dst), 0);
156+
}
157+
108158
TEST_F(Util, utf8_to_wstr_unicode) {
109159
#undef SRC_STR
110160
#undef SRC_AID
@@ -121,8 +171,6 @@ TEST_F(Util, utf8_to_wstr_unicode) {
121171
free(dst_wc.str);
122172
}
123173

124-
125-
126174
TEST_F(Util, ascii_c2w2c)
127175
{
128176
#undef SRC_STR

0 commit comments

Comments
 (0)