[CF-Devel] Inventory bug SC_VERSINO 1024

Fri Oct 27 19:25:56 CDT 2000

Michael Toennies wrote:
>

     >

      Hi

     >

     >

      If i understand the what the code do, then they drop out 2 strings in the

     >

      name

     >

      string. The first is the name for 1 object, the 2nd for 2 or more.

Correct.

>

     >

      I found the way, to copy the whole string very bad.

     >

     >

      I prefer to patch the bug in this way.

     >

     >

      Because i write now the first png-only client (i will extend him with some

     >

      fency features like shadow chaches where you can store hicolors or splitted

     >

      grafik sets, wait for my first doc), simply add then SC_VERSION to 1025.

 I am not clear on what the problem is.  The server side of the code is designed
to deal with long items.  Now there could be a bug there, but it should be easy
to fix.

 Upping the version for your client doesn't really work - all the servers and
clients have to agree with what the numbers mean.  Down the road, the unix/gtk
clients may go to version 1026 if stuff is added, with corresponding change to
the server, at which point any special code gets lost.

>

     >

      Then change the name string to this:

     >

     >

      "<head name of single item> \0 <head name of mult. items> \0 <tail of

     >

      item>\0"

     >

     >

      This is smart and safe a few bytes in the package.

 Depending on the name of the item.  Some items (like a +3 sword), have the same
head for both single and multiple - only the last character changes for plural
items.

 This is not a simple thing to analyze, as it could be argued that the longer
item names are more likely to have the plural at the front, so you save bytes,
and conversely, it could be argued those items are less common, so you really
don't anything, etc...

 And inventory is probably not a big place to worry about bandwidth anyways.  So
saving a few bytes doesn't mean much.

>

     >

      Don't forget the last \0, in the 1024 code the 2nd string has none and when

     >

      your

     >

      nlen is bad, there is no safe way to parse the package. So, its a real fatal

     >

      error.

 If nlen is bad, that should be fixed.  Generally speaking, in all cases, if
packet length is incorrect, you are basically screwed, because the data is sent
in binary and there is no really good way to resync.

 I think I see the bug, and it is trivially easy to fix - one line, and it
doesn't change the protocol at all. To be precise, the code currently looks
like:

               int len;
                char *item_p;

                strncpy(item_n,query_base_name(tmp, 0),127);
                item_n[127]=0;
                len=strlen(item_n);
                item_p=query_base_name(tmp, 1);
(add this)	item_p[127]=0;
                strncpy(item_n+len+1, item_p, 127);
                item_n[254]=0;
                len += strlen(item_p) + 1; <-- this is bad
                SockList_AddChar(&sl, len);
                memcpy(sl.buf+sl.len, item_n, len);
                sl.len += len;

 since item_p is not null terminate at max length, the addition doesn't work.
The addition of that line should make everything work properly.

 Is there a handy item out there to test this against?  I'm not sure how many
items have super long names.