Reading from socket in C giving weird output

Question

I'm having trouble reading from a socket. The code I'm using is below, sometimes it works just fine, but at other times, it just prints some unreadable characters, or some random readable ones... is there a better way?

    char* answer = (char*) malloc(1024);
    int answerLength = 0;
    char prevChar = 0;
    char newChar = 0;
    while (answerLength < 1024 && read(sock, &newChar, 1) > 0 ) {

            if (newChar == '\n' && prevChar == '\r') {
                    break;
            }
            printf("%d\n", answerLength);
            answer[ answerLength ] = newChar;
            answerLength++;

            prevChar = newChar;
    }
    printf("%s\n", answer);

in C, do not cast the returned value from malloc() (and family of functions). 'magic' numbers like that '1024' should be #defined to greatly ease the burden of debugging and later maintenance. The #define name (go programming style says: all caps) should be indicative of what the 1024 means — user3629249, Apr 29 '15 at 03:32
regarding this line: 'if (newChar == '\n' && prevChar == '\r') {' the OS/compiler will handle the difference between OS line endings, (some OS use only linefeed, some OS use only carriage return, some OS use a multi char string. The compiler will handle the details, so the code only needs to use '\n' — user3629249, Apr 29 '15 at 03:36
suggest using calloc() rather than malloc, so the allocated memory is 'already' initialized to all ''\0' so there is no worry about adding a null terminator to the input string. BTW: when answerLength is 1023, should stop, not add another char, as adding the char will result in the 'answer' buffer having no room for the NUL string termination char — user3629249, Apr 29 '15 at 03:42

Havenard · Answer 1 · 2015-04-27T21:26:17.560

3

Strings in C must be null-terminated, which means they must have a symbol \0 as the last character.

Since you don't guarantee that it will happen anywhere in your code, answer may be padded with memory garbage next to the data you read.

To make sure it won't happen, use:

answer[answerLength] = '\0';
printf("%s\n", answer);

Also, you could just read() the whole thing straight to answer, you don't need that pointless loop:

int len;
while (len = read(sock, &answer[answerLength], 1024 - answerLength))
    answerLength += len;
answer[answerLength] = '\0';
printf("%s\n", answer);

edited Apr 27 '15 at 21:26

answered Apr 27 '15 at 21:22

Havenard

27,022
5
36
62

Agreed. It's important to remember that the memory returned from malloc will be filled with undefined contents; it's not zeroed unless the code explicitly zeros it, thus, since the code doesn't manually null terminate, the `printf` could print any amount of garbage at the end depending solely on what was in that memory previously. – antiduh Apr 27 '15 at 21:25
1

Or don't use `printf()` but a function that takes the length as an argument, the key problem is that `printf()` needs to determine where the string ends, since it does not find `'\0'`, it keeps reading through the garbage... – Iharob Al Asimi Apr 27 '15 at 21:25
@iharob: `printf()` *can* take the length as argument: `printf(%.*s\n", answerLength, answer)` – caf Apr 27 '15 at 21:49
Your single `read()` isn't equivalent to the original - the original read up to the first `"\r\n"` sequence, 1023 characters or error (whichever happens first) , but yours just takes whatever it gets in one read, which might not include a `"\r\n"` sequence at all, or might include more than one. – caf Apr 27 '15 at 21:54

score 2 · Answer 2 · edited Apr 27 '15 at 21:26

2

The data you read isn't terminated with a '\0' character, so you can't treat is as a string.

edited Apr 27 '15 at 21:26

Iharob Al Asimi

52,653
6
59
97

answered Apr 27 '15 at 21:23

Andrew Henle

32,625
3
24
56

Serdalis · Accepted Answer · 2015-04-27T21:38:41.200

Your char array is not guaranteed to be null terminated. This means that the printf may print more than just what is in your array since it looks for a null termination to stop outputting characters.

You also don't initialise the allocated memory before using it, which is bad practice since the memory can contain random garbage.

To get the code to work better and hopefully fix your problem, you should do the following:

char* answer = malloc(1024 + 1); /* add extra byte for null terminator */
/* other variables the same */

memset( answer, '\0', 1024 + 1 ); /* initialise memory before use */
while (answerLength < 1024 && read(sock, &newChar, 1) > 0 ) {
    /* loop is the same */
}
printf("%s\n", answer);

There is also an argument in printf which will tell it to print a certain number of characters. Like so:

printf( "%.*s\n", answerLength, answer );

Please remove the cast from `malloc()`, read [this](http://stackoverflow.com/a/605858/1983495) — Iharob Al Asimi, Apr 27 '15 at 21:27

Reading from socket in C giving weird output

3 Answers3