I'll think about this, it sounds like a good idea indeed.
I'll think about this, it sounds like a good idea indeed.
Modifed CInsim
i was:
0. include insim v5
1. change send_mtc
size =len + (4 - len%4) Reason : see below
2. add "swith case" in send_packet to detect ISP_BTN and ISP_MTC and do functions.
3. change return type to bool
4. you can call send_packet(&pack, errmsg) or send_packet(&pack)
PS. i do for me, if MaKaKaZo likes it, he can correct the mistakes and put this code
0. include insim v5
1. change send_mtc
size =len + (4 - len%4) Reason : see below
2. add "swith case" in send_packet to detect ISP_BTN and ISP_MTC and do functions.
3. change return type to bool
4. you can call send_packet(&pack, errmsg) or send_packet(&pack)
PS. i do for me, if MaKaKaZo likes it, he can correct the mistakes and put this code
MaKaKaZo, i was test you method
(((124-1)/4)+1)*4 = 124 but need 128 because lfs said "last byte must be zero"
but 124 + (4 - 124%4) = 128
(((124-1)/4)+1)*4 = 124 but need 128 because lfs said "last byte must be zero"
but 124 + (4 - 124%4) = 128
Really the longest string can only be 127 character long, the 128th character must be NULL (0). If you have a string that's 124 characters long, then you have to use the string length of 128. If you have a string that's 123 characters long, then you can use a string length of 124, so long as 124 is equal to NULL (0). Really it's just a nice way to clip the size of a string and keep the packet size down.
I used
because in the insim documentation it doesn't say anywhere that the "Text" field for the IS_BTN packet must end with a zero:
That's why I used that. I understand that in IS_BTN packets you don't need to add a terminating zero byte. In IS_MTC the situation is different as it states that the string must be zero terminated.
(1+((text_len-1)/4))*4because in the insim documentation it doesn't say anywhere that the "Text" field for the IS_BTN packet must end with a zero:
char Text[240]; // 0 to 240 characters of textThat's why I used that. I understand that in IS_BTN packets you don't need to add a terminating zero byte. In IS_MTC the situation is different as it states that the string must be zero terminated.
MaKaKaZo, just see my CInsim, may be you like it
(DarkTimes)
DELETED
by DarkTimes : Ignore me
The BTN text sometimes does need a null terminator. If you send a BTN with text exactly 240 characters long (no trailing '\0') then LFS just discards the button with no error message. This was a bug I had to track down recently in InSim.NET.
Edit: It occurs to me now that this may actually be a bug in LFS.
I think I never did a hard test on this matter, I just went on with what the doc said. But now I think that I have been sending buttons without a null terminator all the time since I included the send_button() function (aproximately one of every four buttons that contain text). The text on this buttons shows up just fine, so maybe it's a bug that happens with texts 240 chars long.
I can't do a test right now, but i'm pretty sure that a button containing "1234" will send only those four bytes as the Text field (in the latest CInSim version), and they will show right without any need of a null terminator. I don't know about 240 chars...
Yes, I've tested it.
If you send a button that's a multiple of four then it gets displayed, despite lacking a trailing zero. However if the text is exactly 240 characters then it does not get displayed, unless you replace the last char with a '\0'.
Here is a Python script I used to test the issue. I send three buttons, first eight chars long (no trailing zero), second 240 chars long (no trailing zero), third 239 + trailing zero. Only the first and third buttons get displayed. There is no error message in LFS about the button, it seems it's just discarded.
If you send a button that's a multiple of four then it gets displayed, despite lacking a trailing zero. However if the text is exactly 240 characters then it does not get displayed, unless you replace the last char with a '\0'.
Here is a Python script I used to test the issue. I send three buttons, first eight chars long (no trailing zero), second 240 chars long (no trailing zero), third 239 + trailing zero. Only the first and third buttons get displayed. There is no error message in LFS about the button, it seems it's just discarded.
import socket
import struct
sd = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
sd.connect(('127.0.0.1', 29999))
# Init InSim.
isi = struct.pack('4B2HBcH15sx15sx',
44, # Size
1, # Type
0, # ReqI
0, # Zero
0, # UDPPort
0, # Flags
0, # Sp0
'\x00', # Prefix
0, # Interval
'', # Admin
'Test',) # IName
sd.send(isi)
# Send button with eight chars of text (displayed)
btn = struct.pack('12B8s',
20, # Size
45, # Type
1, # ReqI
255, # UCID
1, # ClickID
0, # Inst
0, # BStyle
0, # TypeIn
20, # L
20, # T
40, # W
10, # H
'12345678',) # Eight chars long
sd.send(btn)
# Create a str 240 chars long.
text = ''.join(['a' for i in xrange(0, 240)]) # Create str.
# Send button with exactly 240 chars (not displayed)
btn = struct.pack('12B240s',
252, # Size
45, # Type
1, # ReqI
255, # UCID
2, # ClickID
0, # Inst
0, # BStyle
0, # TypeIn
20, # L
40, # T
40, # W
10, # H
text,) # 240 chars long
sd.send(btn)
# Create str 240 chars with trailing zero
text = ''.join(['a' for i in xrange(0, 239)]) + '\x00'
# Send button 240 chars with trailing zero (displayed)
btn = struct.pack('12B240s',
252, # Size
45, # Type
1, # ReqI
255, # UCID
3, # ClickID
0, # Inst
0, # BStyle
0, # TypeIn
20, # L
60, # T
40, # W
10, # H
text,) # 240 chars with null terminator
sd.send(btn)
while True:
pass
I think you should open a new thread to report this and maybe Scawen will have a look at it. The behaviour should be persistent regardless of the text size.
Perhaps there's something wrong with the way I tested it, but this bug seems to occur when a size of IS_BTN is 252 bytes and the text is NOT 239 chars long with zero terminator.
Sending text up to 236 chars (248 bytes long IS_BTN) works regardless if I zero terminate it or not. If the packet size is 252, then the only packet that doesn't get dropped is one that contains zero terminated 239 chars long text like DarkTimes posted.
I used this to check
EDIT: Just found out that if I memset the memory to 0 instead of 1, InSim behaves just like DarkTimes posted.
Sending text up to 236 chars (248 bytes long IS_BTN) works regardless if I zero terminate it or not. If the packet size is 252, then the only packet that doesn't get dropped is one that contains zero terminated 239 chars long text like DarkTimes posted.
I used this to check
struct IS_BTN btn_pack;
memset(&btn_pack, 1, sizeof(IS_BTN)); //Memset to 1 to not have a zero terminator at the end "automatically"
btn_pack.Type = ISP_BTN;
btn_pack.ReqI = 1;
btn_pack.UCID = 255;
btn_pack.ClickID = 1;
btn_pack.L = 1;
btn_pack.T = 1;
btn_pack.W = 240;
btn_pack.H = 10;
char* text = /****/
memcpy(btn_pack.Text, text, strlen(text)); //Copy just the text, NOT the terminator
btn_pack.Text[strlen(text)] = '\0';
insim.send_button(&btn_pack, strlen(text) + 1); //Modified func form CInsim, second parameter is the length of the text to send
EDIT: Just found out that if I memset the memory to 0 instead of 1, InSim behaves just like DarkTimes posted.
I confirm the IS_BTN had a bug which I have now fixed. It is not supposed to require a trailing zero - that is because bandwidth is very important when sending a lot of buttons, so this helps you to keep button size to a minimum.
The IS_BTN should allow text up to 240 characters (with no zero) and I've fixed that in my version. I'm aiming to get to a compatible update on Friday.
The bandwidth saving issue doesn't really apply with messages, and LFS stores screen messages internally in a 128 byte buffer with terminating zeros, so I'll leave the message packets requiring a terminating zero, although I understand that seems inconsistent with the buttons which do not require a terminating zero.
The IS_BTN should allow text up to 240 characters (with no zero) and I've fixed that in my version. I'm aiming to get to a compatible update on Friday.
The bandwidth saving issue doesn't really apply with messages, and LFS stores screen messages internally in a 128 byte buffer with terminating zeros, so I'll leave the message packets requiring a terminating zero, although I understand that seems inconsistent with the buttons which do not require a terminating zero.
Thanks, Scawen! 
Nice, that one is sorted then, and I won't have to change my code for sending buttons 
I think I'll redo the send_packet() function so that depending on the packet type -which is always the second byte of the struct/data to be sent- it will then call to separate functions in case it's a button, a mtc or otherwise (generic packet). I'm not feeling like doing it today anyway. What do you people think?
I think I'll redo the send_packet() function so that depending on the packet type -which is always the second byte of the struct/data to be sent- it will then call to separate functions in case it's a button, a mtc or otherwise (generic packet). I'm not feeling like doing it today anyway. What do you people think?
You had something like this in mind?
int CInsim::send_packet(void* s_packet)
{
pthread_mutex_lock (&ismutex);
//Detect packet type
switch(*((unsigned char*)s_packet+1))
{
case ISP_BTN:
{
struct IS_BTN* pack = (struct IS_BTN*)s_packet;
unsigned char text_len = strlen(pack->Text);
/*TODO: Should we truncate the string if it's too long
* or should we just discard the packet?
* If we discard the packet, perhaps another
* return code should be used. */
if(text_len > IS_BTN_MAXTLEN)
return -1;
unsigned char text2send;
unsigned char remdr = text_len % 4;
if(remdr == 0)
text2send = text_len;
else
text2send = text_len + 4 - remdr;
pack->Size = IS_BTN_HDRSIZE + text2send;
}
break;
case ISP_MTC:
{
struct IS_MTC* pack = (struct IS_MTC*)s_packet;
unsigned char text_len = strlen(pack->Text);
//Same as above
if(text_len > IS_MTC_MAXTLEN)
return -1;
unsigned char text2send = text_len + 4 - text_len % 4;
pack->Size = IS_MTC_HDRSIZE + text2send;
}
break;
default:
break;
}
if (send(sock, (const char *)s_packet, *((unsigned char*)s_packet), 0) < 0)
{
pthread_mutex_unlock (&ismutex);
return -1;
}
pthread_mutex_unlock (&ismutex);
return 0;
}
Yes, that's the main idea. I'd keep the send_button() probably for compatibility reasons.
About checking text size, I don't want to do it. When you declare an IS_BTN struct the Text field is statically set to 240 chars long. In theory you shouldn't be able to send a button packet with a text longer than that, unless you modify the definition of the struct in insim.h or you do something nasty in your application. So I consider there's no need to worry about that
Can you overflow the struct within C, considering your not using a NULL terminator?
You can overflow pretty much anything in C, something like char c[3]; memcpy(c, "XXXXXXXX", 8); is perfectly legal as far as compiler is concerned. Executing it would probably lead to segfault, so MaKaKaZo is right about not needing any sort of check for overflows. Even if such malformed packet gets through, LFS discards it anyway.
char c[3];
memcpy(c, "XXXXXXXX", 8);That's insane! Surly we can do better then this! Don't get me wrong I understand that C++ (and C in general) is very fast, but something like this should really be checked at compile time. No good can from from that code being allowed to execute.
(DarkTimes)
DELETED
by DarkTimes
When you try to copy a source that is larger then a destination.
[EDIT] Damn it DarkTimes, stop deleting your posts. It's making me look like I'm talking to myself!
So, type softly and use a big stack?
[EDIT] Damn it DarkTimes, stop deleting your posts. It's making me look like I'm talking to myself!
So, type softly and use a big stack?
memcpy takes a pointer dst and will fill the memory from that location onwards with size bytes of src. The compiler doesn't know (well, it does, but by passing it to memcpy you're casting it to a void*) that the pointer points to an array (strictly speaking, to the beginning of it) of 3 chars, it's simply an address.
That's the C way though, C++ offers a much safer way already in the form of the STL containers. They take care of everything memory-related (allocation, reallocation on resize, freeing on destruction).
doesn't overflow, is properly initialised (empty string) and doesn't leak.
€: Obviously there are less error-prone ways in C as well, didn't mean to make C look completely hopeless :P
That's the C way though, C++ offers a much safer way already in the form of the STL containers. They take care of everything memory-related (allocation, reallocation on resize, freeing on destruction).
std::string c;
c = "XXXXXXXX";doesn't overflow, is properly initialised (empty string) and doesn't leak.
€: Obviously there are less error-prone ways in C as well, didn't mean to make C look completely hopeless :P
Remind me to learn C++ then, because C is a big scary monster!
The stupid example above could be easily caught by the compiler, but sizes of data a program works with are usually dynamic, so there'd have to be some runtime checking to prevent invalid memory access. Consider a typical example of lame coding:
char c[10];
std::cin >> c;
std::cout << c << std::endl;
As long as user doesn't input anything longer than 9 chars, everything is OK and the compiler cannot possibly compensate for a programmer not taking care of this.
C++ has it's own big-scary-monstrous things too BTW
I only delete my posts because they suck and I have no idea what I'm talking about.
Well, that makes two of us, but I leave my suckieness out there (and my foot in my mouth for the whole world to see).
C++ - CInsim - A basic C++ InSim library
(162 posts, started )
FGED GREDG RDFGDR GSFDG