Transfer array of integers over a socket in C/C++

Question

This answer shows methods for sending and receiving integers over sockets in c/c++. Based on that I wrote following two functions, sendKInt and receiveKInt to send and receive K integers. But its not working correctly. Maybe my pointer arithmetic is wrong, but I cannot find the error. Please help with that, I am new to c++. Also see sendTill and receiveTill functions.

bool sendKInt(int fd, int num[], int k){ // returns true on success
    int32_t conv[k];
    char *data;
    for (int i=0; i<k; i++){
        conv[i] = htonl(num[i]);
        char *data = (char*)conv;
    }
    return sendTill(sizeof(int32_t)*k, fd, data);
}

bool receiveKInt(int fd, int *num, int k){ // returns true on success
    int32_t ret[k];
    int toReturn = receiveTill(sizeof(int32_t)*k,fd,(char*)ret);
    for (int i=0; i<k; i++){
        num[i] = ntohl(ret[i]);
    }
    return toReturn;
}

bool sendTill(int size, int fd, char* dataU){ // dataU - dataUser pointer is NULL then create your own buffer and send random thing // returns true on successs
    char *data;
    int left = size;
    int rc;
    if(dataU == NULL){
        data = (char*)malloc(size);
    }else{
        data = dataU;
    }
    while (left) {
        rc = write(fd, data + size - left, left);
        if (rc < 0){
            cout <<"-1 in sendTill \n";
            return false;
        }
        left -= rc;
    }
    return true;
}

bool receiveTill(int size, int fd, char* data){ // just give a char pointer after allocating size // returns true on success
    int ret;
    int left = size;
    while (left) {
        ret = read(fd, data + size - left, left);
        if (ret < 0){
            cout <<"-1 in receiveTill \n";
            return false;
        }
        left -= ret;
    }
    return true;
}

seeing setup code for `fd` would be helpful. Also, invest in some Wireshark (or Message Analyzer if this is loopback traffic on Windows). Seeing network traffic (or a lack of it) helps narrow these problems down. — yano, Sep 07 '16 at 20:33
At the receiving side, how is it known how many integers to receive? I think sender has to send the size of the array (*k*) first. — Arun, Sep 07 '16 at 20:33
@Arun I am using them with same k on both sides, for test case. So thats not an issue. — PHcoDer, Sep 07 '16 at 20:36
This looks uncomfortably close to C, rather than (modern) C++... — Jesper Juhl, Sep 07 '16 at 20:40
@JesperJuhl, yes thats correct. I am using these as API calls for other c++ program. However what do you want to convey by this? — PHcoDer, Sep 07 '16 at 20:43
I assume you're not getting errors from your `rc` return ..? Does your program appear to hang or does it execute to completion? — yano, Sep 07 '16 at 20:49
@PHcoDer what I want to convey is that seeing manual memory management in modern C++ code is more often than not unneeded and a code smell and source of bugs. And even if you really *have* to, then `new`/`delete` should be used (since they deal correctly with ctors/dtors), never `malloc`/`free`. The fact that this code uses raw pointers rather than smart pointers or just `std::` containers, tells me that there are bound to be problems ahead.. that's all - the code looks like C, C is error prone, modern C++14 has better alternatives.. — Jesper Juhl, Sep 07 '16 at 20:55

user4581301 · Answer 1 · 2016-09-07T21:06:33.050

4

This

char *data;

is not the same as

char *data = (char*)conv;

You have two datas, one inside the for loop which is assigned and one outside that is not so

return sendTill(sizeof(int32_t)*k, fd, data);

winds up sending the outer, uninitialized, data. Crom only knows what will happen.

Fortunately, there is no point to doing

char *data = (char*)conv;

a few hundred times, so

return sendTill(sizeof(int32_t)*k, fd, (char*)conv);

looks like it will work. Sadly it won't in C++.

int32_t conv[k];

is invalid C++. k is not known at compile time, so this requires a non-standard Variable Length Array compiler extension. Some compilers will allow this syntax, but it has some downsides (common implementations easily overrun the stack and horribly messes with the behaviour of sizeof) and won't compile on all compilers. Really sucks if you need to port the code to other platforms or the code is being submitted to someone who will evaluate the code and uses, for example, Visual Studio.

C++11 has std::vector::data so you can

bool sendKInt(int fd, int num[], int k){ // returns true on success
    std::vector<int32_t> conv(k);
    for (int i=0; i<k; i++){
        conv[i] = htonl(num[i]);
    }
    return sendTill(sizeof(int32_t)*k, fd, (char *)conv.data());
}

edited Sep 07 '16 at 21:06

answered Sep 07 '16 at 20:52

user4581301

33,082
7
33
54

Thanks! Then same would be issue with receiveIntK too? – PHcoDer Sep 07 '16 at 20:59
@PHcoDer Yes. `ret[k]` will have the same problem. – user4581301 Sep 07 '16 at 21:00
But I think doing malloc there will solve the problem? – PHcoDer Sep 07 '16 at 21:01
3

`malloc` should be used as a last resort in C++. Prefer `new`. In this case though you could read directly into `num` and `num[i] = ntohl(num[i]);` to save yourself any more allocations. – user4581301 Sep 07 '16 at 21:03
also, the last line throws compilation error (char *)conv::data. Also (char *)conv throws error. – PHcoDer Sep 07 '16 at 21:06
My mistake. I left off the `()` on `conv::data()`. Actually, I screwed it up bigtime. Should be `conv.data()` – user4581301 Sep 07 '16 at 21:07

score 2 · Answer 2 · answered Sep 07 '16 at 20:52

You declare two data variables, the second one hiding the first.

bool sendKInt(int fd, int num[], int k){ // returns true on success
    int32_t conv[k];

    // First "data", points to undefined value
    char *data;
    for (int i=0; i<k; i++){
        conv[i] = htonl(num[i]);

        // local data, points to conv
        // HIDES the first data declared above
        char *data = (char*)conv;
    }

    // uses the only "data" in scope, which points to undefined location
    return sendTill(sizeof(int32_t)*k, fd, data);
}

Christophe · Answer 3 · 2016-09-07T21:39:34.413

Why it doesn't work

It is not clear if you intend from sendKInt():

to send all the data via sendTill(), as suggest by the parameter sizeof(int32_t)*k that you use in the call
to send the data one by one, as your conversion loop converts only a single item

In sendKInt() loop you have two very big mistakes:

you declare a bloc variable char *data in the loop. This variable hides the outside variable with the same name, and its value is lost as soon as you exit the loop.
when you exit the loop you call sendTill() using the outside data variable which is still uninitialized. This is undefined behavior guaranteed !
Note that even if you wouldn't hide the data variable it wouldn't work, because you cast the conv value (i.e. an integer to transmit) into a pointer to a character (e.g. if the interger is 0, you'll use a null poitner). So you would try to send a rogue pointer, so certainly not the value that yo've expected.

How to solve it

I assume C as you use malloc().

bool sendKInt(int fd, int num[], int k){ // returns true on success
    int32_t *data = calloc (sizeof(int32_t), k);  // allocate for everything
    if (data==NULL)                            // allocation failure
        return 0;                               
    for (int i=0; i<k; i++){                   // convert everything 
        data[i] = htonl(num[i]);
    }
    bool rc = sendTill(sizeof(int32_t)*k, fd, (char*)data);
    free(data);                               // malloc=> free (or you'll leak memory) 
    return rc;                            // now that memory is freed, return the result
}

Note that you have to review the rest of the code according to this new logic. Don't forget to free memory that you allocate in a function in order to avoid memory leakage.

Thanks!. Only edit needed would be use of calloc in receiveKInt. Right? — PHcoDer, Sep 07 '16 at 21:12
@PHcoDer the important thing is that data is initialized with allocated memory for the conversion to take place. So no change of this pointer within the loop (second edit). And no return with a function call as you need to free the allocated data before. Oh ! I just notice that I forgot to return the value. i'll edit — Christophe, Sep 07 '16 at 21:38

score 0 · Accepted Answer · answered Sep 07 '16 at 22:01

A nicely working code:

bool sendKInt(int fd, int num[], int k){ // returns true on success
    std::vector<int32_t> conv(k);
    for (int i=0; i<k; i++){
        conv[i] = htonl(num[i]);
    }
    return sendTill(sizeof(int32_t)*k, fd, (char *)conv.data());
}

bool receiveKInt(int fd, int *num, int k){ // returns true on success
    int32_t *ret = (int32_t*)calloc (sizeof(int32_t), k);
    bool toReturn = receiveTill(sizeof(int32_t)*k,fd,(char*)ret);
    for (int i=0; i<k; i++){
        num[i] = ntohl(ret[i]);
    }
    free(ret);
    return toReturn;
}

bool sendTill(int size, int fd, char* dataU){ // dataU - dataUser pointer is NULL then create your own buffer and send random thing // returns true on successs
    char *data;
    int left = size;
    int rc;
    if(dataU == NULL){
        data = (char*)malloc(size);
    }else{
        data = dataU;
    }
    while (left) {
        rc = write(fd, data + size - left, left);
        if (rc < 0){
            cout <<"-1 in sendTill \n";
            return false;
        }
        left -= rc;
    }
    return true;
}

bool receiveTill(int size, int fd, char* data){ // just give a char pointer after allocating size // returns true on success
    int ret;
    int left = size;
    while (left) {
        ret = read(fd, data + size - left, left);
        if (ret < 0){
            cout <<"-1 in receiveTill \n";
            return false;
        }
        left -= ret;
    }
    return true;
}

A few things that should be fixed in this answer. 1) Code only answers lead to [Cargo Cult Programmers](https://en.wikipedia.org/wiki/Cargo_cult_programming). I recommend marking what you changed, why, and how it's better than what OP had. 2) Switches back and forth between C and C++ styles of programming. While most C code works just fine in C++, the reverse is not true and C in C++ often results in sub-optimal code. 3) `sendTill` does not `free` its `malloc`ed data. — user4581301, Sep 07 '16 at 23:04

Transfer array of integers over a socket in C/C++

4 Answers4