Bug 1916 - String truncated when appended to buffer in TdsComm
Summary: String truncated when appended to buffer in TdsComm
Alias: None
Product: Class Libraries
Classification: Mono
Component: System.Data ()
Version: master
Hardware: PC Linux
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
Depends on:
Reported: 2011-11-07 10:18 UTC by Neale Ferguson
Modified: 2017-09-01 09:30 UTC (History)
2 users (show)

Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:

Comment 1 Neale Ferguson 2011-11-08 11:18:54 UTC
If I make the following change to TdsComm.cs:

--- a/mcs/class/Mono.Data.Tds/Mono.Data.Tds.Protocol/TdsComm.cs
+++ b/mcs/class/Mono.Data.Tds/Mono.Data.Tds.Protocol/TdsComm.cs
@@ -391,28 +391,9 @@ namespace Mono.Data.Tds.Protocol {
                        if (tdsVersion < TdsVersion.tds70) { 
                                Append (encoder.GetBytes (s));
                        } else {
-                               int cindex = 0, index;
-                               int ssize = sizeof (short);
-                               int lenToWrite = s.Length * ssize;
-                               // if nextOutBufferLength points to the last buffer in outBuffer, 
-                               // we would get a DivisionByZero while calculating remBufLen
-                               if (outBufferLength - nextOutBufferIndex < ssize)
-                                       SendIfFull (ssize);
-                               int remBufLen = outBufferLength - nextOutBufferIndex;
-                               int count = lenToWrite/remBufLen;
-                               if (lenToWrite % remBufLen > 0)
-                                       count++;
-                               for (int i = 0; i < count; i++) {
-                                       index = System.Math.Min (remBufLen/ssize, lenToWrite/ssize);
-                                       for (int j = 0; j < index*ssize; j+=2, cindex++)
-                                               AppendInternal ((short)s[cindex]);
-                                       lenToWrite -= index*ssize;
-                                       // Just make sure to flush the buffer
-                                       SendIfFull ((lenToWrite+1)*ssize);
+                               for (int i = 0; i < s.Length; i++) {
+                                       SendIfFull (sizeof(short));
+                                       AppendInternal ((short)s[i]);

The problem disappears. I am unsure why the logic for adding a string to the buffer is so complex. If a single byte string is being converted into 16bits unicode and it's okay to split such a string on a 16bit boundary then the check should just be to see if there is enough room for the next character before appending it, if not send, reset the pointer, and then append. Am I missing something of importance in the way strings should be handled?
Comment 2 Neale Ferguson 2011-11-08 13:59:52 UTC
The original code in Append(string) used to check the size of the entire string before the
appending begins:

       SendIfFull (s.Length * 2);
       for (int i = 0; i < s.Length; i++)
            AppendInternal ((short)s[i]);

A commit log entry said this was changed to avoid possible ArgumentOutOfRangeException conditions.

What I'm doing is checking before each converted byte is being put in the buffer. What I think is that the SendIfFull(sizeof(short)); will prevent us going past the range of the output buffer.

In fact using the original logic, in the case where a buffer is relatively small (e.g. a system using an MTU of 576) and a long string (of say, 270 bytes or more) it will never be possible to put the string into the buffer.

The criticism that may be leveled against my mechanism is the performance hit from checking before appending every character of the string. However, the code used to calculate remaining lengths including calls to System.Math.Min probably means that my method is not that much of an imposition. With a suitable optimization level it should get inlined (shouldn't it?).
Comment 3 Gonzalo Paniagua Javier 2011-11-08 14:44:35 UTC
Neale, I would go ahead and check in the patch. The bug fixed by the patch is a greater concern than the slight performance hit.
Comment 4 Neale Ferguson 2011-11-08 14:53:57 UTC
Comment 5 Gonzalo Paniagua Javier 2011-11-08 15:06:01 UTC
Thanks, Neale!