what is wrong with the following while loop?

Question

I have a c winsock code section where a client receives a comma delimited stream of file fingerprints as shown below. I need to extract the fingerprints from the stream using strtok_s() in a while loop. My problem is most of the time the client does not extract the exact number of fingerprints sent from the server, even though the data received(observed by debugging) is exactly what the server sent. What am I missing here?

recv_size = recv(clnt_sock, fp_buf, BUF_LEN, 0);
            received_fp_size += recv_size;
        if (0 != (last_string_len = recv_size % 33))
            strncpy(last_string, &fp_buf[(recv_size - last_string_len)], last_string_len);//
        while (recv_size > 0)
        {
            unique_fp = strtok_s(fp_buf, ",", &strtk);
        k:
            while (unique_fp != NULL)
            {
                memcpy(unique_fp_buf[unique_files_count], unique_fp, 32);
                unique_fp = strtok_s(NULL, ",", &strtk);
                unique_files_count++;

            }


            recv_size = recv(clnt_sock, fp_buf, BUF_LEN, 0);
            received_fp_size += recv_size;
            if (last_string_len > 0)
            {
                unique_fp = strtok_s(fp_buf, ",", &strtk);
                strncat_s(last_string, unique_fp, strlen(unique_fp));
                memcpy(unique_fp, last_string, 32);
                last_string_len = 0;
                goto k;
            }

        }

The reason behind the if (0 != (last_string_len = recv_size % 33)) line is; The server sends a multiple of 33 byte strings(32 for the fingerprint and 1 for the coma demlimiter)

Answer 1

One problem is that you never check that fp_buf actually contains a complete token. For instance if the first call only receives 20 bytes, your code will fail by copying a partial fingerprint.

I think another problem is here:

memcpy(unique_fp, last_string, 32);

Seems you are copying into the receive buffer and therefore overwrites some data that you haven't processed yet. Further, you may overwrite a token.

Maybe you actually wanted:

memcpy(unique_fp_buf[unique_files_count], last_string, 32);
                                          ^^^^^^^^^^^
unique_fp = strtok_s(NULL, ",", &strtk);
unique_files_count++;

Besides that I think you are making the code much more complicated than needed. The use of a goto kind of tell you that your design is wrong.

Instead of using a last_string you could do:

1) Call recv
2) Process all complete fingerprints
3) Copy the remainder (i.e. the last partial fingerprint) to the start of `fp_buf`
4) Call `recv` with an offset into `fp_buf`
5) Repeat from step 2 (i.e. use a while loop - don't use goto

Step 3 could be something like:

recv_size = recv(clnt_sock, fp_buf + length_of_remainder , BUF_LEN -  length_of_remainder, 0);

In that way you don't have to handle the last_string stuff