server-client injecting random char's?

i created a simple server-client application in which simple I/O is sent and received. but apparently, when the client sends a packet and the server receives it, the buffer to which the data is received is injected with random characters, usually caused because of there not being a string termination (\0), but there is.

here's the client:

Code:

#include <stdlib.h>
#include <stdio.h>
#include <windows.h>
#include "Headers\\sock.h"

int main()
{
    sock s;
    char data[512];

    startup();

    opensock(&s);
    printf("Connection[%d]\n",sockconnect(&s, "127.0.0.1", 43594));

    while(1) {
        scanf("%s", data);
        data[strlen(data)] = '\0';
        if (send((SOCKET) s, data, strlen(data), 0) == -1)
           break;
        else
            printf("SENT %s\n", data);

        int i;
        for (i = 0; i < strlen(data); i++)
            data[i] = 0;
    }

    return 0;
}

here is the server:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <windows.h>
#include "Headers\\sock.h"

typedef struct {
    sockinf *sin;
    int threadnum;
} sockinf_;

void handle(void *s)
{
    sockinf_ *sn = (sockinf_ *)s;
    char buff[512];

    printf("Client[%d]: Connected from %s\n", sn->threadnum, getip(sn->sin->addr));

    while(1) {
        if (read(&(sn->sin->s), buff, 512) > 0) {
            printf("Length %d\n", strlen(buff));
            buff[strlen(buff)] = '\0';
            printf("Client[%d]: %s\n", sn->threadnum, buff); 
            int i;
            for (i = 0; i < strlen(buff); i++)
                buff[i] = 0;
        }
        Sleep(1000);
    }
}

int main()
{
    sock s;

    printf("%d\n",startup());

    printf("%d\n", opensock(&s));
    printf("%d\n",sockbind(&s, 43594));
    if (start(&s, 100) != -1)
        printf("Started server..\n");

    int i;
    
    for (i = 1;; i++) {
        sockinf_ ss;
        sockinf ii = acceptsock(&s);
        ss.sin = &ii;
        ss.threadnum = i;
        _beginthread(handle, 0, (void *)&ss);
    }       
}

by the way, it has nothing to do with the 'sock' header i provide since it directly uses the winsock functions and returns the same return code.

> data[strlen(data)] = '\0';
This does nothing - think about it.

> if (send((SOCKET) s, data, strlen(data), 0) == -1)
There are many other results as well.
In particular, send() may only be partially successful.


> _beginthread(handle, 0, (void *)&ss);
Passing a pointer to a local is really bad news. Existing threads end up with trashed data the next time around the loop you go.

> if (read(&(sn->sin->s), buff, 512) > 0)
Do something like

Code:

n = read(&(sn->sin->s), buff, sizeof(buff)-1)
if ( n > 0 ) {
    buff[n] = '\0';
} else if ( n == 0 ) {
  // closed
} else {
  // error
}

You can't use strlen() as you've no idea if a \0 has been sent, and read() won't add one for you just in case.

Originally Posted by Salem

> data[strlen(data)] = '\0';
This does nothing - think about it.

it wouldn't work because strlen() looks for \0, but i do not know of any other way of doing this.

Originally Posted by Salem

> if (send((SOCKET) s, data, strlen(data), 0) == -1)
There are many other results as well.
In particular, send() may only be partially successful.

i am very aware of that. but right now i am only checking to see if it returns -1, because when it does, it usually means the socket is disconnected.

Originally Posted by Salem

> _beginthread(handle, 0, (void *)&ss);
Passing a pointer to a local is really bad news. Existing threads end up with trashed data the next time around the loop you go.

would something like: '_beginthread(handle, 0, ((void *))(sockinf_ *)&ss);' be better?

Originally Posted by Salem

> if (read(&(sn->sin->s), buff, 512) > 0)
Do something like

Code:

n = read(&(sn->sin->s), buff, sizeof(buff)-1)
if ( n > 0 ) {
    buff[n] = '\0';
} else if ( n == 0 ) {
  // closed
} else {
  // error
}

You can't use strlen() as you've no idea if a \0 has been sent, and read() won't add one for you just in case.

here's my read function:

Code:

int read(sock *s, char *buff, int max)
{
    int i = recv((SOCKET) *s, buff, max, 0);
    if (i > 0)
        buff[i] = '\0';

    return i;
}

but it's still not terminating the string...

> it wouldn't work because strlen() looks for \0
Yes, and you replace the \0 with another \0 - what have you achieved?

> would something like: '_beginthread(handle, 0, ((void *))(sockinf_ *)&ss);' be better?
allocating using malloc would be better, then each thread is assured a unique copy.

> but it's still not terminating the string...
1. Don't hijack the name of an existing function like read().
2. You pass sizeof(buff), which means if you actually fill the buffer, the '\0' goes out of bounds.

Here's another pit you've dug for yourself with strlen()
> for (i = 0; i < strlen(buff); i++)
> buff[i] = 0;
Because you've already done buff[0] = 0;, the strlen on the second time around the loop will return 0, and the remainder of the buffer will be untouched.

IMO, all this buffer clearing is a waste of time. If you're making good use of all the return results, you should be fine.