server-client injecting random char's?

This is a discussion on server-client injecting random char's? within the C Programming forums, part of the General Programming Boards category; i created a simple server-client application in which simple I/O is sent and received. but apparently, when the client sends ...

  1. #1
    Registered User
    Join Date
    Dec 2008
    Posts
    104

    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.

  2. #2
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,490
    > 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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  3. #3
    Registered User
    Join Date
    Dec 2008
    Posts
    104
    Quote Originally Posted by Salem View Post
    > 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.

    Quote Originally Posted by Salem View Post
    > 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.


    Quote Originally Posted by Salem View Post
    > _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?

    Quote Originally Posted by Salem View Post
    > 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...

  4. #4
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,490
    > 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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. server client problem
    By rudeboy in forum Networking/Device Communication
    Replies: 5
    Last Post: 05-17-2008, 12:41 AM
  2. silly client server question
    By geek@02 in forum Windows Programming
    Replies: 10
    Last Post: 12-03-2007, 04:23 AM
  3. Replies: 2
    Last Post: 11-23-2007, 01:10 AM
  4. Multi-Threaded Server Help
    By jonswits20 in forum C# Programming
    Replies: 5
    Last Post: 04-17-2007, 11:05 PM
  5. Client application having problem receiving from server side?
    By dp_76 in forum Networking/Device Communication
    Replies: 2
    Last Post: 08-04-2005, 02:58 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21