Wrong IP and port from server socket

Question

When I write a code like below, the reported ip and port of the peer is correct:

int main(){
    int server = socket(AF_INET,SOCK_STREAM,0);
    struct sockaddr_in addr;    
    addr.sin_family = AF_INET;
    addr.sin_port = htons(8080);
    addr.sin_addr.s_addr = htonl(INADDR_ANY);
    bind(server,(struct sockaddr *)&addr,sizeof addr);
    listen(server,5);
    struct sockaddr_storage inc_adrs;
    socklen_t inc_len;
    int client = accept(server, (struct sockaddr *) &inc_adrs,&inc_len);
    struct sockaddr_in *inc_adr = (struct sockaddr_in *) &inc_adrs;    
    char ip [INET_ADDRSTRLEN];
    inet_ntop(AF_INET,&(inc_adr->sin_addr),ip,INET_ADDRSTRLEN);
    printf("Connection from %s port %d\n",ip,ntohs(inc_adr->sin_port));
    return 0;
}

And for example I connect to it using nc on my local machine :

#nc 127.0.0.1 8080

And the ouput is:

Connection from 127.0.0.1 port 55112

But if I just put them in another block like "if" or "while" the program reports wrong ip and port:

int main(){
    if (1){
        int server = socket(AF_INET,SOCK_STREAM,0);
        struct sockaddr_in addr;    
        addr.sin_family = AF_INET;
        addr.sin_port = htons(8080);
        addr.sin_addr.s_addr = htonl(INADDR_ANY);
        bind(server,(struct sockaddr *)&addr,sizeof addr);
        listen(server,5);
        struct sockaddr_storage inc_adrs;
        socklen_t inc_len;
        int client = accept(server, (struct sockaddr *) &inc_adrs,&inc_len);
        struct sockaddr_in *inc_adr = (struct sockaddr_in *) &inc_adrs;    
        char ip [INET_ADDRSTRLEN];
        inet_ntop(AF_INET,&(inc_adr->sin_addr),ip,INET_ADDRSTRLEN);
        printf("Connection from %s port %d\n",ip,ntohs(inc_adr->sin_port));
        return 0;
    }
}

Sample output:

Connection from 253.127.0.0 port 32323

I compile it with "gcc code.c -o code" on linux x64. It is very strange and I don't know how it is possible. Any idea?

no need to use sockaddr_storage, immediately use a sockaddr_in. Check the values inside that struct before using inet_ntop. — Philip Stuyck, Aug 21 '15 at 17:36
Also read this : http://stackoverflow.com/questions/20130415/casting-sockaddr-storage-as-sockaddr-in-for-inet-ntop Don't use inet_ntop. — Philip Stuyck, Aug 21 '15 at 17:38
1. Use SO_REUSEADDR: http://stackoverflow.com/questions/24194961/how-do-i-use-setsockoptso-reuseaddr 2. check return values from `socket`, `bind`, `listen`, `accept`. If one of these fail you get garbage down the code. — fukanchik, Aug 21 '15 at 17:41
@PhilipStuyck I did what you said the problem persists. This seems impossible that "if block" changes the behavior of program. Why is the problem happening while in "if" — CS., Aug 21 '15 at 17:44
@fukanchik This code has been simplified to take less space. The main code contains error checking and also SO_REUSEADDR with no error reported. But what is happening is making me crazy — CS., Aug 21 '15 at 17:46
@CS. So this is not the real code? Then I'm not going to bother debugging it. — melpomene, Aug 21 '15 at 18:26
@fukanchik Same thing happens. This code behaves differently while in the "if" block. — CS., Aug 21 '15 at 18:39
@melpomene I tried also this code and what I said happens. I can solve the problem by using different APIs but the only thing I wanna know is WHY is this happening? It seems an implementation fault in APIs and heap related or I don't know — CS., Aug 21 '15 at 18:41
@CS. Aside from the missing initialization of `inc_len` (which is *required*, that is a bug on your part), and the complete lack of error handling (which you should never omit), the rest of the code is fine. The only way I can see an added `if` block breaking the code is if your compiler is broken and producing bad codegen. The more likely culprit is that an uninitialized `inc_len` picks up a random value from the stack and causes `accept()` to fail, and you then pass garbage to `inet_ntop()`. Even your original code without the `if` block is susceptible to that issue. — Remy Lebeau, Aug 21 '15 at 20:46
@CS. fix your code bug, add error checking, and try again. If the problem continues, edit your question to show your latest code, you are likely still doing something wrong with it. This is too simple a problem for it to likely be caused by a compiler codegen bug. — Remy Lebeau, Aug 21 '15 at 20:48
@RemyLebeau As I said I fixed the code myself but wanted to know what is happening behind the scene. As currently I don't have time to use a debugger and analyze the program assembly I asked the question here. I should mention also the this code (which you call buggy) always works correctly outside this block or in another function but main. — CS., Aug 21 '15 at 21:05
@CS.: You are passing garbage into `accept()` to begin with. Anything beyond that is undefined behavior since this code is not doing ANY error handling. If you don't want to spend time properly debugging the code, then at least include the value of `inc_len` in the app's output, you will likely see a pattern when things fail. — Remy Lebeau, Aug 21 '15 at 21:56

fukanchik · Accepted Answer · 2015-08-21T21:18:16.177

Okay, looks like the problem is in how you use man 2 accept:

The addrlen argument is a value-result argument: the caller must initialize it to contain the size (in bytes) of the structure pointed to by addr; on return it will contain the actual size of the peer address.

The returned address is truncated if the buffer provided is too small; in this case, addrlen will return a value greater than was supplied to the call.

With the following fix plus SO_REUSEADDR, your code works fine:

    struct sockaddr_storage inc_adrs;
    socklen_t inc_len = sizeof( inc_adrs );

Also, do not forget to check result values from socket, bind, listen, accept.

./a.out
Connection from 127.0.0.1 port 11111
---
nc 127.0.0.1 8080 -p 11111

Connection from 192.168.0.1 port 11111
---
nc 192.168.0.1 8080 -p 11111

You are using struct sockaddr_storage inc_adrs; after accept can possibly (depending on random value in inc_len):

copy full value into inc_adrs
copy incomplete value into inc_adrs
never touch into inc_adrs at all if inc_len happened to be zero.

For example in my case with if inc_len is zero. inc_adrs remains untouched and contains some garbage:

inc_adrs = {ss_family = 0, __ss_align = 234832925920,
            __ss_padding = "@\300\377\377\377\177\000\000G\004I\255\066\000\000\000X...

this garbage depends on how the compiler allocates values on stack and what previous code (including glibc) will do to your stack.

For example same value without if:

1: inc_len = 54 <<--- this one is unitialized!!!!
2: inc_adrs = {ss_family = 49240, __ss_align = 4294967296,
             __ss_padding = "\000\000\000\000\000\000\000\000\034\00

as you can see inc_len happens to be 54 which is more than enough for accept to save sockaddr_in and hence this case works as if everything is okay.

The difference between if and not-if cases:

-       leaq    -52(%rbp), %rdx
-       leaq    -208(%rbp), %rcx
+       leaq    -180(%rbp), %rdx
+       leaq    -176(%rbp), %rcx
        movl    -4(%rbp), %eax
        movq    %rcx, %rsi
        movl    %eax, %edi
        call    accept

So it works correctly only by chance. You can simulate the same with random:

#include <stdlib.h>

srand(time(NULL));
...
socklen_t inc_len = random() % (sizeof(struct sockaddr_in)*2)

this will fill inc_adrs 50% of time and 5% not touch it.

Thanks for replying. But I didn't get the answer I was looking for. As I said in the comments I could solve the problem by other ways but I wanted to know WHY this happens. Why this code works outside of "if" or any other scope like "while" but not inside. As this must NOT make a problem. I wanted to know what is happening here — CS., Aug 21 '15 at 20:11
That is what debuggers are for. Have you tried debugging the code and inspecting the value of `inc_len` before calling `accept()`, and inspecting the content of `inc_adrs` and the value of `inc_len` after `accept()` exits? Did you try inspecting what you are passing to `inet_ntop()`? Garbage in, garbage out. — Remy Lebeau, Aug 21 '15 at 20:49
@RemyLebeau No I have not. But how can it depend on a single "if" block in the way outside of "if" it always works correctly? — CS., Aug 21 '15 at 21:06
After you edited your answer I agree more and I suspect that without "if" most of the time (if not always) the stack is filled in a way which inc_len gets an "acceptable" value! — CS., Aug 21 '15 at 21:26
@fukanchik Yes the only thing is that what causes the program not to perform randomly ? As I said every time I get correct output while code is outside if block. Seems it needs more assembly analysis. Thank you — CS., Aug 21 '15 at 21:49
@CS: "*For example in my case with `if` `inc_len` is zero. **`inc_adrs` remains untouched and contains some garbage**:*" - ALWAYS INITIALIZE YOUR VARIABLES! You MUST initialize `inc_len` before calling `accept()` so it knows how large `inc_adrs` really is. But even then, you should get in the habit of zeroing out structures before filling them. Had you done that, your output would more likely have been `Connection from 0.0.0.0 port 0` instead of random garbage. That would have been your first clue that `accept()` was not populating `inc_adrs`. — Remy Lebeau, Aug 21 '15 at 22:01

Wrong IP and port from server socket

1 Answers1