Skip to content

Commit

Permalink
fix buffer underrun for wrong received buffer size (#199)
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
bpintea committed Dec 2, 2019
1 parent 6e645fd commit 3ed053d
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 11 deletions.
18 changes: 10 additions & 8 deletions driver/util.c
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ size_t json_escape_overlapping(char *str, size_t inlen, size_t outlen)
* wstr according to avaialble space and indicates the available bytes to copy
* back into provided buffer (if not NULL).
*/
SQLRETURN write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
SQLRETURN TEST_API write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
SQLSMALLINT /*B*/avail, SQLSMALLINT /*B*/*usedp)
{
size_t wide_avail;
Expand All @@ -533,8 +533,8 @@ SQLRETURN write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
"len; out-len @0x%p.", src->cnt, LWSTR(src), dest, avail, usedp);

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

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

if (dest) {
/* needs to be multiple of SQLWCHAR units (2 on Win) */
if (avail % sizeof(SQLWCHAR)) {
ERRH(hnd, "invalid buffer length provided: %d.", avail);
if (avail < 0 || avail % sizeof(SQLWCHAR)) {
ERRH(hnd, "invalid buffer length provided: %hd.", avail);
RET_HDIAGS(hnd, SQL_STATE_HY090);
} else {
wide_avail = avail/sizeof(SQLWCHAR);
}

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

INFOH(hnd, "not enough buffer size to write required string (plus "
"terminator): `" LWPD "` [%zu]; available: %zu.",
"terminator): `" LWPDL "` [%zu]; available: %zu.",
LWSTR(src), src->cnt, wide_avail);
RET_HDIAGS(hnd, SQL_STATE_01004);
} else {
Expand Down
2 changes: 1 addition & 1 deletion driver/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ size_t json_escape_overlapping(char *str, size_t inlen, size_t outlen);
* wstr according to avaialble space and indicates the available bytes to copy
* back into provided buffer (if not NULL).
*/
SQLRETURN write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
SQLRETURN TEST_API write_wstr(SQLHANDLE hnd, SQLWCHAR *dest, wstr_st *src,
SQLSMALLINT /*B*/avail, SQLSMALLINT /*B*/*usedp);

/*
Expand Down
52 changes: 50 additions & 2 deletions test/test_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

extern "C" {
#include "util.h"
#include "handles.h"
} // extern C

#include <gtest/gtest.h>
Expand Down Expand Up @@ -105,6 +106,55 @@ TEST_F(Util, wstr_to_utf8_no_nts) {
free(dst.str);
}

TEST_F(Util, write_wstr_invalid_avail) {
wstr_st src = WSTR_INIT(SRC_STR);
SQLWCHAR dst[sizeof(SRC_STR)];
esodbc_env_st env = {0};
SQLSMALLINT used;

SQLRETURN ret = write_wstr(&env, dst, &src, sizeof(*dst) + 1, &used);
ASSERT_FALSE(SQL_SUCCEEDED(ret));
}

TEST_F(Util, write_wstr_0_avail) {
wstr_st src = WSTR_INIT(SRC_STR);
SQLWCHAR dst[sizeof(SRC_STR)];
esodbc_env_st env = {0};
SQLSMALLINT used;

SQLRETURN ret = write_wstr(&env, dst, &src, /*avail*/0, &used);
assert(SQL_SUCCEEDED(ret));
ASSERT_EQ(used, src.cnt * sizeof(*dst));
}

TEST_F(Util, write_wstr_trunc) {
wstr_st src = WSTR_INIT(SRC_STR);
SQLWCHAR dst[sizeof(SRC_STR)];
esodbc_env_st env = {0};
SQLSMALLINT used;

SQLRETURN ret = write_wstr(&env, dst, &src,
(SQLSMALLINT)((src.cnt - 1) * sizeof(*dst)), &used);
assert(SQL_SUCCEEDED(ret));
ASSERT_EQ(used, src.cnt * sizeof(*dst));
ASSERT_EQ(dst[src.cnt - 2], L'\0');
ASSERT_EQ(wcsncmp(src.str, dst, src.cnt - 2), 0);
}

TEST_F(Util, write_wstr_copy) {
wstr_st src = WSTR_INIT(SRC_STR);
SQLWCHAR dst[sizeof(SRC_STR)];
esodbc_env_st env = {0};
SQLSMALLINT used;

SQLRETURN ret = write_wstr(&env, dst, &src, (SQLSMALLINT)sizeof(dst),
&used);
assert(SQL_SUCCEEDED(ret));
ASSERT_EQ(used, src.cnt * sizeof(*dst));
ASSERT_EQ(dst[src.cnt], L'\0');
ASSERT_EQ(wcscmp(src.str, dst), 0);
}

TEST_F(Util, utf8_to_wstr_unicode) {
#undef SRC_STR
#undef SRC_AID
Expand All @@ -121,8 +171,6 @@ TEST_F(Util, utf8_to_wstr_unicode) {
free(dst_wc.str);
}



TEST_F(Util, ascii_c2w2c)
{
#undef SRC_STR
Expand Down

0 comments on commit 3ed053d

Please sign in to comment.