FreeTDS Development Group

["Ucko, Aaron (NIH/NLM/NCBI) [E]" <ucko AT ncbi.nlm.nih.gov>]

Thanks for being open to these changes!

I did get this first reply, but was having a busy week and wanted to take a 
closer look at the commits in which I patched unit tests before following up. 
 Most of my commits to unit tests turned out to be pure formalities or setup 
fixes, but there is one commit that could well be an issue here:

    cs_convert, dbconvert: Truncate data as needed; update unit tests.
    
    * Automatically truncate strings that would overflow their destinations,
      mainly for the sake of bulk insertions.  Notify the client at low
      severity (as a warning, essentially).  Update upstream unit tests
      (ctlib t0006, dblib t0019) accordingly.
    * Fully honor *BINARY and IMAGE data buffer size limits in cs_convert.

Running these tests against Sybase libraries (12.5, 15.7, and 16.0, with 
identical results in all cases), I see that these changes do go against 
upstream, but that there are already several other differences on this front 
for each API (details attached).  I'll pull this commit out, at least for now.

FWIW, there was also one commit for which I had to patch 
src/tds/unittests/convert.c, but not any higher-level unit tests:

    ctlib: Implement full Unicode support for TDS 7+ (MSSQL).
    
    * Introduce CS_NLONGCHAR_TYPE and CS_NVARCHAR_TYPE, both corresponding to
      SYBNVARCHAR (along with CS_UNICHAR_TYPE); have ct_param automatically
      promote CS_{LONG,VAR}CHAR_TYPE to CS_N{LONG,VAR}CHAR_TYPE for TDS 7+
      when non-ASCII characters appear in the data.  (Unnecessary use of
      Unicode types can make parameterized queries against 8-bit text
      columns run very slowly.)
    * ctlib/cs.c (cs_convert): Formally support Unicode destination types
      (SYBNVARCHAR and SYBNTEXT).
    * ctlib/ct.c (paraminfoalloc): When setting maximum length automatically,
      account for possible conversion to UCS-2.
    * tds/convert.c (tds_convert), tds/tds_willconvert.*: Formally support
      Unicode types, giving type 103 (0x67) the benefit of the doubt as
      SYBNVARCHAR rather than SYBSENSITIVITY.
    * tds/data.c (tds_set_param_type): Map SYBNVARCHAR to XSYBNVARCHAR for
      TDS 7+.
    * tds/unittests/convert.c: Accommodate Unicode types, converting ASCII
      input strings to (native-endian, is this correct?) UCS-2 as needed.

-- Aaron Ucko <ucko at ncbi>, NCBI C++ Toolkit core development group

-----Original Message-----
From: Frediano Ziglio [mailto:freddy77 AT gmail.com] 
Sent: Wednesday, June 20, 2018 04:58
To: FreeTDS Development Group <freetds AT lists.ibiblio.org>
Cc: Ucko, Aaron (NIH/NLM/NCBI) [E] <ucko AT ncbi.nlm.nih.gov>
Subject: Re: [freetds] Reintegrating years of local FreeTDS changes

Hi,
   a pull request seems good.
Didn't read the full list, I expect some to be small, some other larger.
I usually keep the vendor behavior if I can.

Regards,
   Frediano


2018-06-06 22:28 GMT+01:00 Ucko, Aaron (NIH/NLM/NCBI) [E] via FreeTDS
<freetds AT lists.ibiblio.org>:
> Long story short, I am the primary maintainer of a convenience (aka
> vendorized) copy of FreeTDS's client libraries that has accumulated some
> local changes over the years.  I have now forward-ported these changes
> to your master branch and would belatedly like to submit them for your
> consideration, both because they may also be of interest to other users
> and to make future releases easier to integrate.
>
> Some of the developers who contributed to (the original versions of)
> these changes have moved on to other jobs, but we were all working on
> behalf of the US federal government, which cannot hold copyright, so
> there shouldn't be any legal concerns; I'm just citing them in (so far
> unpublished) Git metadata to give credit where credit's due.
>
> I tested these changes via your test suite against both Microsoft SQL
> Server and Sybase ASE, and observed no regressions, just one failure
> that turned out to be preexisting (specifically, of the ODBC utf8_2 test
> against Microsoft SQL Server).  We currently have Microsoft SQL Server
> 2014 and 2017 and Sybase ASE 15.5 and 16.0 on hand, and additionally
> tested many of these changes against older server versions too.  That
> said, in a few cases (some predating my involvement with this code), we
> made deliberate changes to semantics.  In such cases, I've updated the
> test suite accordingly.  However, if you don't agree with these changes,
> I can of course instead arrange to make them conditional in some
> fashion; please let me know whether I should do so for any or all of
> these changes.
>
> I have grouped these changes into 59 patches.  Please don't take this
> count as criticism of your code, which works well for the most part;
> we've just been sitting on our changes for entirely too long and
> gradually accumulating more along the way.  Also, I suspect our needs
> are somewhat atypical -- we use the CT-LIB interface, and do so via a
> home-grown abstraction library rather than directly from application
> code.
>
> How would you like me to proceed?  Should I send the patches here, or
> put them online through a GitHub fork?  In the latter case, should I
> proceed to issue a formal pull request?
>
> Here's a quick rundown:
>
> * Fix miscellaneous configuration-specific issues, mostly on Windows.
> * Enable automatic TDS protocol version detection.
> * thread.h (tds_mutex_trylock): Disallow recursive locking on Windows.
> * tds_iconv: Accommodate FreeBSD/Citrus iconv.
> * net.c (tds_socket_write): Don't let fake short writes break headers.
> * Fix unit tests that manipulate raw network connections.
> * cancel.c: Skip return_data tests with Sybase 15+.
> * numeric.c: Don't force TDS 5.0 (DBVERSION_100).
> * threadsafe.c (tds_getservice): Set AI_CANONNAME where available.
> * dblib.c: Honor dbsetversion; improve dbcolinfo and dbwritetext.
> * odbc.c (SQLSetEnvAttr): Uncomment SQL_ATTR_OUTPUT_NTS-handling code.
> * ct.c (ct_describe): Populate datafmt->format per Valgrind.
> * connectparams.c (odbc_parse_connect_string): Handle client_charset.
> * ct.c (ct_connect): Set client_charset in addition to server_charset.
> * tds_process_tokens: For DONEPROC, always restore rows_affected.
> * Implement blk_textxfer, over a resumable tds_bcp_send_record.
> * query.c: Handle return codes of tds_put_data and tds_put_data_info.
> * net.c: Improve handling of certain error conditions.
> * ctlib: Handle CS_TIMEOUT and CS_LOGIN_TIMEOUT.
> * ct.c (_ct_get_server_type): Handle CS_LONGCHAR_TYPE.
> * blk.c (_blk_get_col_data): Use on-server size for destfmt.maxlength.
> * ct.c (ct_cursor): Predict cursor type for CS_CURSOR_DECLARE.
> * Fix server timeout handling and ctlib severity propagation.
> * Don't log directly to stdout or stderr.
> * blk.c: Handle CS_BLK_CANCEL in blk_done.
> * cs_convert, dbconvert: Truncate data as needed; update unit tests.
> * ct.c (ct_data_info): Encode/decode CS_IODESC datatype field.
> * ctlib: Work with non-NULL empty strings if the server supports them.
> * ctlib: Implement full Unicode support for TDS 7+ (MSSQL).
> * Improve support for TDS 5 (Sybase), mainly around bulk transfers.
> * Improve TDS 5 (Sybase) LONGBINARY and LONGCHAR handling.
> * Address miscellaneous compiler warnings.
> * Report system errors' descriptions to ctlib clients.
> * mem.c (tds_deinit_bcpinfo): Free and reset current_row for xfers in.
> * read.c (tds_get_n): Avoid potential hangs on short replies.
> * tds_checks.c (tds_check_column_extra): Skip size check for BLOBs.
> * Address Clang static analyzer errors, many long-standing.
> * Improve CTLIB support for TDS 7.2+.
> * cs.c (cs_convert): Fix *resultlen for conversions to SYBIMAGE.
> * bulk.c (tds_bcp_send_record): Reject invalid NULL data.
> * ctlib: Optimize blob conversion for bulk transfers in.
> * Declare and implement blk_sethints(_ver95).
> * data.c (tds_generic_row_len): Account for column_varint_size.
> * ct.c (ct_get_data): Populate cmd->iodesc->name more efficiently.
> * mem.c (tds_init_login): Enable use_utf16 by default.
> * query.c (tds_fix_column_size): Allow full size for output columns.
> * login.c (SET_FIELD_DSTR, for tds7_send_login): Cap lengths at 128.
> * tds/u.t./common.c (try_tds_login): Pass base name to tds_set_app.
> * Address more issues caught by LLVM/Clang scan-build.
> * odbc.c (odbc_process_tokens): Pre-zero done_flags per Valgrind.
> * threadsafe.c (tds_get_homedir/_WIN32): Initialize path.
> * data.c (tds_set_param_type): Under TDS 7.1+, send SYBBIT as SYBINT1.
> * ct.c (paraminfoalloc): Report long variable-width values.
> * packet.c (tds_write_packet): fix a TDS_ADDITIONAL_SPACE corner case.
> * tds5_process_dyn_result2: Don't trust Sybase to accept blob params.
> * net.c (tds_close_socket): Don't try to close invalid sockets.
> * stream.c (tds_datain_stream_read): Bail if tds_get_n returns NULL.
> * login.c (tds_connect): Skip non-TCP tries to connect to a host:port.
> * Address gaps in ctlib support for new MS date/time types.
>
> --
> Aaron Ucko <ucko at ncbi>, NCBI C++ Toolkit core development group
>
> _______________________________________________
> FreeTDS mailing list
> FreeTDS AT lists.ibiblio.org
> https://lists.ibiblio.org/mailman/listinfo/freetds
--- ct_t0006-unpatched-syb15.7.out      2018-06-25 10:05:59.815445000 -0400
+++ ct_t0006-patched-syb15.7.out        2018-06-25 10:05:59.710473000 -0400
@@ -1,39 +1,51 @@
 Test result length failed (ret=0 len=2)
 line: 158
   DO_TEST(CS_INT test = 32768; CS_SMALLINT test2 = 12345,
           CS_INT_TYPE,&test,sizeof(test),
           CS_SMALLINT_TYPE,sizeof(test2),
           CS_FAIL,NULL,0);
 Test result length failed (ret=0 len=2)
 line: 160
   DO_TEST(CS_INT test = -32769; CS_SMALLINT test2 = 12345,
           CS_INT_TYPE,&test,sizeof(test),
           CS_SMALLINT_TYPE,sizeof(test2),
           CS_FAIL,NULL,0);
 Test result length failed (ret=0 len=1)
 line: 171
   DO_TEST(CS_INT test = 256; CS_TINYINT test2 = 1,
           CS_INT_TYPE,&test,sizeof(test),
           CS_TINYINT_TYPE,sizeof(test2),
           CS_FAIL,NULL,0);
 Test result length failed (ret=0 len=1)
 line: 173
   DO_TEST(CS_INT test = -1; CS_TINYINT test2 = 1,
           CS_INT_TYPE,&test,sizeof(test),
           CS_TINYINT_TYPE,sizeof(test2),
           CS_FAIL,NULL,0);
 Test result length failed (ret=0 len=4)
 line: 202
   DO_TEST(CS_INT test = 1234678; CS_MONEY4 test2 = { 1234678},
           CS_INT_TYPE,&test,sizeof(test),
           CS_MONEY4_TYPE,sizeof(test2),
           CS_FAIL,NULL,0);
 Test result data failed (ret=1 len=258)
 line: 264
   DO_TEST(CS_INT test = 12345,
           CS_INT_TYPE,&test,sizeof(test),
           CS_VARCHAR_TYPE,sizeof(test2),
           CS_SUCCEED,&test2,sizeof(test2));
-ct_t0006-unpatched.c: Testing conversion
+Test result failed (ret=0 len=2)
+line: 293
+  DO_TEST(CS_CHAR test[] = "abc"; CS_CHAR test2[] = "ab",
+          CS_CHAR_TYPE,test,3,
+          CS_CHAR_TYPE,2,
+          CS_SUCCEED,test2,2);
+Test result failed (ret=0 len=6)
+line: 298
+  DO_TEST(CS_CHAR test[] = "abcdef"; CS_CHAR test2[] = "616263",
+          CS_BINARY_TYPE,test,6,
+          CS_CHAR_TYPE,6,
+          CS_SUCCEED,test2,6);
+../freetds-upstream/src/ctlib/unittests/t0006.c: Testing conversion
 05 00 31 32 33 34 35 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
 05 00 31 32 33 34 35 00 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 17 
--- db_t0019-unpatched-syb15.7.out      2018-06-25 10:06:00.052341000 -0400
+++ db_t0019-patched-syb15.7.out        2018-06-25 10:05:59.946455000 -0400
@@ -1,47 +1,57 @@
 OK: anticipated error 20049 (Data-conversion resulted in overflow.) arrived
 OK: anticipated error 20049 (Data-conversion resulted in overflow.) arrived
 
-line: 100 test: (SYBBINARY, "", 0, SYBBINARY, 6)
+line: 95 test: (SYBCHAR, "ciao  ", 6, SYBCHAR, 4)
+ciao******
+len=-1 63 69 61 6F 2A 2A 2A 2A 2A 2A
+failed :( should be 'len=4 63 69 61 6F 2A 2A 2A 2A 2A 2A'
+
+line: 97 test: (SYBCHAR, "ciao\0\0", 6, SYBCHAR, 4)
+ciao******
+len=-1 63 69 61 6F 2A 2A 2A 2A 2A 2A
+failed :( should be 'len=4 63 69 61 6F 2A 2A 2A 2A 2A 2A'
+
+line: 102 test: (SYBBINARY, "", 0, SYBBINARY, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 101 test: (SYBVARBINARY, "", 0, SYBBINARY, 6)
+line: 103 test: (SYBVARBINARY, "", 0, SYBBINARY, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 102 test: (SYBIMAGE, "", 0, SYBBINARY, 6)
+line: 104 test: (SYBIMAGE, "", 0, SYBBINARY, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 103 test: (SYBBINARY, "", 0, SYBVARBINARY, 6)
+line: 105 test: (SYBBINARY, "", 0, SYBVARBINARY, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 104 test: (SYBVARBINARY, "", 0, SYBVARBINARY, 6)
+line: 106 test: (SYBVARBINARY, "", 0, SYBVARBINARY, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 105 test: (SYBIMAGE, "", 0, SYBVARBINARY, 6)
+line: 107 test: (SYBIMAGE, "", 0, SYBVARBINARY, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 106 test: (SYBBINARY, "", 0, SYBIMAGE, 6)
+line: 108 test: (SYBBINARY, "", 0, SYBIMAGE, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 107 test: (SYBVARBINARY, "", 0, SYBIMAGE, 6)
+line: 109 test: (SYBVARBINARY, "", 0, SYBIMAGE, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'
 
-line: 108 test: (SYBIMAGE, "", 0, SYBIMAGE, 6)
+line: 110 test: (SYBIMAGE, "", 0, SYBIMAGE, 6)
 ......****
 len=0 00 00 00 00 00 00 2A 2A 2A 2A
 failed :( should be 'len=6 00 00 00 00 00 00 2A 2A 2A 2A'