Bugs in "Network Communications in 2009" article

rlebeau · Post by **rlebeau** » Fri Feb 13, 2009 12:44 pm

In the January 2009 "Network Communication in C++ Builder 2009" article, George talks about the issue of backwards compatibility of his TClientSocket/TServerSocket code in BCB 2009 and earlier. Although most of his article is correct regarding how to write code that works in all versions of BCB, his TEdit::OnKeyDown code example is not compatible with BCB 2009 because the TEdit::Text property returns an UnicodeString now instead of an AnsiString. The code needs to be more like the following:

Code: Select all

void __fastcall TForm1::Edit1KeyDown(TObject *Sender, Char &Key, TShiftState Shift)
{
	if(Key == VK_ESCAPE)
	{
		Close();
		return;
	}
	if(Key == VK_RETURN)
	{
		if(!Client->Socket->Connected)
		{
			Edit1->Clear();
			Beep();
			return;
		}

		if( Edit1->GetTextLen() < 1)
		{
			return;
		}

		// in BCB2009, this will convert the Text from
		// Unicode to Ansi, which the server code is
		// expecting anyway (better would be to use
		// UTF-8 on both ends instead so there is no
		// data loss).
		//
		// In earlier versions, this will send the
		// TEdit::Text data as-is.
		//
		AnsiString s = Edit1->Text;

		Client->Socket->SendBuf(s.c_str(), s.Length());
		Memo1->Lines->Add("I'm saying:");
		Memo1->Lines->Add(s);
	}
}

Also, his TClientSocket::OnRead() event handler has a minor buffer overflow bug in it:

Code: Select all

void __fastcall TForm1::ClientRead(TObject *Sender, TCustomWinSocket *Socket)
{
	int length = Socket->ReceiveLength();
	if( length < 1 )
	{
		return;
	}
	char *pszBuffer = new char[length];
	try
	{
		Socket->ReceiveBuf(pszBuffer, length);

		// the data is NOT null-terminated!
		//
		// AnsiString S = pszBuffer;
		AnsiString S(pszBuffer, length);

		Memo1->Lines->Add(S);
	__finally
	{
		delete [] pszBuffer;
	}
}

Something else to keep in mind is that SendText() and SendBuf() are NOT guaranteed to send everything they are given. In the case of SendBuf(), it returns the actual number of bytes written. The caller needs to look at that value and re-call SendBuf() again for any bytes that were not sent the previous time. But I'll leave that as an exercise for the readers to figure out

gtokas · Post by **gtokas** » Fri Feb 13, 2009 2:36 pm

Hello Remy and thanks for the post.
The way you see works as expected...
The story is that a reader informed me and I modified the code the way I proposed and worked...
I will NOT argue with you my friend since I'm sure that in many areas you have a better knowledge not to mention experience due to TEAMB...
ANYWAY:
scktComp.pas the source for TClient and TServer components:
function TCustomWinSocket.SendText(const s: string): Integer;
begin
Result := SendBuf(Pointer(S)^, Length(S));
end;
SendText call internaly SendBuf as I mentioned...

I'm working with send/receive strings since 2001 and using - of course - and hashes never failed...
In fact there are ultra rare cases of failure, because I used logs, that a "packet of data" received corrupted....
I also used double and triple encryptions and again never failed...

With all those I don't mean that I am right and you are not...
If you can provide me examples of failure I will be MORE than happy to document all those in an article together with you....

George Tokas.

rlebeau · Post by **rlebeau** » Fri Feb 13, 2009 10:02 pm

gtokas wrote:SendText call internaly SendBuf as I mentioned...

Yes. I was not arguing that. I was only commenting that SendText() and ReceiveText() are not implemented correctly in 2009. However, looking at them again, I see that SendText() has since been updated to take AnsiString instead of UnicodeString as input now:

Code: Select all

function TCustomWinSocket.SendText(const s: AnsiString): Integer;
begin
  Result := SendBuf(Pointer(S)^, Length(S) * SizeOf(AnsiChar));
end;

However, CodeGear should have used RawByteString instead of AnsiString, so that all types of Ansi-based strings (UTF-8, et) can be sent without having to be converted to the user's default Ansi codepage first.

ReceiveText() is still broken, though:

Code: Select all

function TCustomWinSocket.ReceiveText: string;
begin
  SetLength(Result, ReceiveBuf(Pointer(nil)^, -1));
  SetLength(Result, ReceiveBuf(Pointer(Result)^, Length(Result)));
end;

ReceiveBuf(nil) returns a byte count, not a character count. ReceiveString() is then reading that number of bytes, not characters, into the resulting UnicodeString. So, for example, if the sendig party sends an Ansi-based string of 5 characters/bytes, then ReceiveString() will allocate a UnicodeString of 5 UTF-16 characters (10 bytes) but only read 5 non-Unicode bytes into it. CodeGear should have updated ReceiveString() to return an Ansi/RawByteString instead of a UnicodeString.

gtokas wrote:I'm working with send/receive strings since 2001 and using - of course - and hashes never failed...

If you are relying on SendText() always sending the full text in one go, and ReceiveText() always receiving the full text in one go, then you have been VERY lucky so far, but that is NOT guaranteed behavior, especially for longer strings, and/or if the sockets are used in non-blocking mode.

gtokas · Post by **gtokas** » Sat Feb 14, 2009 1:24 am

>>ReceiveText() is still broken, though:
That was what I meant mentioning "I didn't examine it deeper..." in the article...
My primary concerns were:
1. I forwarded a person from Experts-Exchange to the journal being sure that what I wrote in my articles are still working.
2. That person read them, downloaded the code and tried to work...

He informed me that the result was garbage...
I opened BCB2009 and made the 2 apps server and client...
Client->SendText(Text); had the "Text" contents intact...
Server side Text = Socket->ReceiveText(); "Text" has the exact length but the data were NOT the same...
And therefore I started the workaround....
I don't know if I'm THAT lucky ( or the other word for"longshot" we are using here in Greece ) but I have used a string greater than 1Kb in result, triple encrypted and each encryption hashed, and didn't had problem not only in LAN based but also internet based...
I tested all those server side, on internet configuration, streaching an early ADSL connection (2M/256K) to the limit - more than 99% of internet bandwidth used from other machines in the local network for just download/upload....
BUT just because I think I'm NOT any kind of genius or "know-it-all" if you have any objection or idea I will be happy to write an article together with you...

I have the GREATEST respect for all those people (including you) working and evolving INDY and once I was subscriber to the commercial products even though I never worked with them...

George Tokas.

rlebeau · Post by **rlebeau** » Sat Feb 14, 2009 4:59 pm

gtokas wrote:I don't know if I'm THAT lucky ( or the other word for"longshot" we are using here in Greece ) but I have used a string greater than 1Kb in result, triple encrypted and each encryption hashed, and didn't had problem not only in LAN based but also internet based...

Then you have been very lucky so far. I have had strings be split into multiple transmissions before.

You really should be looking at the return value of SendBuf() so you know how many bytes were actually accepted into the socket, and how many bytes still need re-sending. Also, especially for dynamically-sized data, it is important to send the actual data length in addition to the data, so that the receiver knows exactly how many bytes to expect and can call ReceiveBuf() multiple times if it can't receive it all in a single operation.

For example:

Code: Select all

bool __fastcall SendData(void *Data, int Len)
{
    unsigned char *ptr = (unsigned char*) Data;
    while( Len > 0 )
    {
        int sent = ...SendBuf(ptr, Len);
        if( sent > 0 )
        {
            ptr += sent;
            Len -= sent;
        }
        else if( (sent == 0) || (WSAGetLastError() != WSAEWOULDBLOCK) )
            return false;
    }
    return true;
}

bool __fastcall SendText(const RawByteString &S)
{
    int len = S.Length();
    if( SendData(&len, sizeof(int)) )
        return SendData(S.c_str(), len);
    return false;
}

bool __fastcall ReadData(void *Data, int Len)
{
    unsigned char *ptr = (unsigned char*) Data;
    while( Len > 0 )
    {
        int read = ...ReceiveBuf(ptr, Len);
        if( read > 0 )
        {
            ptr += read;
            Len -= read;
        }
        else if( (read == 0) || (WSAGetLastError() != WSAEWOULDBLOCK) )
            return false;
    }
    return true;
}

RawByteString __fastcall ReadText()
{
    int len = 0;
    if( ReadData(&len, sizeof(int)) )
    {
        RawByteString S;
        S.SetLength(len);

        if( ReadData(S.c_str(), len) )
            return S;
    }
    return "";
}

gtokas · Post by **gtokas** » Sun Feb 15, 2009 6:12 am

Hi Remy,
You are right.
Even though I didn't had ANY problem in my history working with sockets, that DOESN'T mean safeguards as you describe in the example shouldn't be present...
In next months article I wiil add all those BUT I will add and your name to the article.

George Tokas.