SegFault on memcpy and source and destination are defined and initialized

Question

I am getting segFault by copying to initialized pointer. this is relevant code

*pay = (char *)malloc(sizeof(t1) + 1020 /*1000=payload*/);
memset(*pay, 0, sizeof(t1) + 1020);
struct ethhdr *eth = (struct ethhdr *) *pay;
struct iphdr *ip = (struct iphdr *) (*pay + sizeof(struct ethhdr));
struct tcphdr *tcp = (struct tcphdr *) (*pay + sizeof(struct ethhdr) + sizeof(struct iphdr));

and I did

 memcpy(eth->h_dest, p->h_source, sizeof(eth->h_source));

But above line is causing a segfault. I am assigning h_source like this in other thread memcpy((p)->h_source, received_packet_eth->h_source, sizeof(eth->h_source));

h_source is inside struct struct packets *p this is how its definition struct packets *p I am initializing and assigning in other thread, see above. So struct packets *p is not an uninitialized pointer. And I am sure it is being allocated when the segFault occurs.

unsigned char h_source[ETH_ALEN];   /* source ether addr    */

Thanks for any help

Update This is the full code of the function

void get_payload_to_send(struct packets *p, char **pay)
{
    printf("%s --->>> %s", p->ip_source, p->ip_dest);
    int t1;
    *pay = (char *)malloc(sizeof(t1) + 1020 /*1000=payload*/);
    memset(*pay, 0, sizeof(t1) + 1020);
    struct ethhdr *eth = (struct ethhdr *) *pay;
    struct iphdr *ip = (struct iphdr *) (*pay + sizeof(struct ethhdr));
    struct tcphdr *tcp = (struct tcphdr *) (*pay + sizeof(struct ethhdr) + sizeof(struct iphdr));

    //if (p->h_proto == ntohs(p->h_proto) == ETH_P_IP)
    {
        if (p->syn == 1 && p->ack == 0 && p->fin == 0)
        {
            ///Ethernet
            //memcpy(eth->h_dest, p->h_source, sizeof(eth->h_source));

            eth->h_proto = htonl(ETH_P_IP);
            //memcpy(eth->h_source, p->h_dest, sizeof(eth->h_dest));
            ////IP

            // memcpy(ip->saddr, inet_addr(p->ip_dest), sizeof(ip->saddr));
            struct sockaddr_in s1, s2;
            s1.sin_family = AF_INET;
            s1.sin_port = htons(80);
            s1.sin_addr.s_addr = inet_addr(p->ip_source);

            s2.sin_family = AF_INET;
            s2.sin_port = htons(5009);
            s2.sin_addr.s_addr = inet_addr(p->ip_dest);
            //ip->saddr = s1.sin_addr.s_addr;
            sleep(2);
            ip->daddr = s2.sin_addr.s_addr;
            char c[20];
            memcpy(ip->saddr, inet_addr(p->ip_dest), sizeof(c) - 1);
            ip->ihl = 5;
            ip->version = 4;
            ip->tos = 0;
            ip->tot_len = sizeof(t1) + 1020;
            srand(1001);
            ip->id = htonl(rand());
            ip->frag_off = 0;
            ip->ttl = 225;
            ip->protocol = IPPROTO_TCP;
            ip->check = 0;
            tcp->syn = 1;
            tcp->ack = 1;

            memcpy(ip->daddr, inet_addr(p->ip_source), sizeof(c) - 1);

            //ip->check = csum((unsigned short *)*pay, ip->tot_len);
        }
        else if (p->syn == 0 && p->ack == 1)
        {

        }
    }
    //printf("%saddr %s -->>", p->ip_source);
    //printf("daddr %s\n", p->ip_dest);
    //populate eth
}

This is the code from where the function call to above function is made

void *task_sender(void *args) {
    struct struct_super_struct *super = (struct struct_super_struct *)args;

    struct packets *p = super->p; //(struct packets *)args;
    printf("Sender\n");
    while (1)
    {
        //printf("got uppder loop SENDER\n");

        pthread_mutex_lock(&mutex);
       
        while (nop == 0)
        {
            pthread_cond_wait(&cond, &mutex);
            //printf("finished waiting in SENDER\n");
        }
        int i = 0;

        printf("NOP is greater than 2 in SENDER %d\n");

        while (nop > 0)
        {
            // check_and_process_connection(super,(p+i));

            //if (p == NULL) break;
            if (strcmp("192.168.10.25", (p+i)->ip_dest) == 0 && (p+i)->syn == 1 && (p+i)->ack == 0)
            {
                //sleep(1);

                //printf("%s\n", (p+i)->ip_dest);
                //if (strcmp((p+i)->ip_dest, "192.168.10.25") == 0)
                {
                    char *pac;
                    struct packets *pt = (p+i);
                    get_payload_to_send((p+i), &pac);
                    struct sockaddr_in temp;
                    struct ethhdr *eth = (struct ethhdr *)pac;
                    struct iphdr *ip = (struct iphdr *)(pac + sizeof(struct ethhdr));
                    struct tcphdr *tcp = (struct tcphdr *)(pac + sizeof(struct ethhdr) + sizeof(struct iphdr));
                    temp.sin_addr.s_addr = ip->saddr;
                    char *source = inet_ntoa(temp.sin_addr);
                    struct sockaddr_in temp1;
                    temp1.sin_addr.s_addr = ip->daddr;
                    char *dest = inet_ntoa(temp1.sin_addr);
                    printf("%s >> %s \n", (p+i)->ip_source, (p+i)->ip_dest);
                    //if (strcmp("192.168.10.25",dest) == 0 && strcmp("192.168.10.25", source) == 0)
                    {
                        ///temp1.sin_addr.s_addr = ip->daddr;
                        printf("should be\n");
                        printf("frm %s to %s syc:%d ack:%d \n",
                               inet_ntoa(temp.sin_addr),
                               inet_ntoa(temp1.sin_addr),
                               tcp->syn, tcp->ack);
                    }
                    /*          
                    printf("__________________________________________________\n");
                    printf("lastop: %d\n", (p+i)->lastop);
                    printf("source: %s %d\n", (p+i)->ip_source, (p+i)->tcp_source);
                    printf("dest: %s %d\n", (p+i)->ip_dest, (p+i)->tcp_dest);
                    printf("syc: %d\n", (p+i)->syn);
                    printf("ack: %d\n", (p+i)->ack);
                    printf("seq: %d\n", (p+i)->seq);
                    printf("ack seq: %d\n", (p+i)->ack_seq);
                    (p+i)->lastop = 0;
                    printf("lastop: %d\n", (p+i)->lastop);
                    */
                }
                //printf("dest port: %d\n", (p+i)->tcp_dest);
            }
            //else { printf("source ip: %s\n", (p+i)->ip_source); printf("dest ip: %s\n", (p+i)->ip_dest); }
            //p++;
            nop--;
            i++;
            //printf("just processed packet SENDER\n");
        }
        //p = NULL;
    
        pthread_mutex_unlock(&mutex);
        nop = 0;
        int s = pthread_cond_signal(&cond);
    }
}

Update 2

struct packets {
    int syn;
    char payload[1000];
//    pthread_mutex_t  mutex;
    int lastop;
    unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
    unsigned char h_source[ETH_ALEN];   /* source ether addr    */
    __be16      h_proto;
    char ip_source[20];
    char ip_dest[20];
    int tcp_source;
    int tcp_dest;
    int seq;
    int ack_seq;

    int ack;
    int fin;
    int rst;
    int window;
    int nop;
    struct ethhdr *eth;
    struct iphdr *ip;
    struct tcphdr *tcp;
}; // *p=NULL;

Note What I found debugging with gdb is that in memcpy(eth->h_dest, p->h_source, sizeof(eth->h_source)); line eth->hdest is NULL

it matters, why. This is not why I am getting segFault. am I? — user786, Apr 04 '21 at 07:08
`pay` passed as argument `pointer to pointer` in side called function I with `*pay=(char *) malloc(sizeof(t1)+1020/*1000=payload*/);` initialized inside called function — user786, Apr 04 '21 at 07:10
I cannot see any obvious problems, so I must suspect there's a race condition. — Tordek, Apr 04 '21 at 07:23
We cannot see any obvious problems either because you have not supplied the data structs or the calling code. How can we tell if your pay pointer is valid? We surely cannot tell if your design is thread/interrupt safe. — Martin James, Apr 04 '21 at 07:29
**Everything in this code is wrong.** We do not see the declarations of ethernet, ip or tcp header but there is no guarantee that you can add the 14 bytes of sizeof ethernet header and land to a location that is aligned properly for an IP header. — Antti Haapala -- Слава Україні, Apr 04 '21 at 07:55
this this its related https://stackoverflow.com/questions/66905392/passing-char-uninitialiezed-pointer-address-and-allocation-and-extraction-of-mem — user786, Apr 04 '21 at 07:57
Ok the problem is that you're **not** introducing another variable. Do not try to "conserve" memory, make another variable of type `char *` to use within that function and you can get rid of those silly `*` everywhere. — Antti Haapala -- Слава Україні, Apr 04 '21 at 08:00
What is this `t1` nonsense? Why you have some random `sizeof (int)` to add to the malloc? None of this makes sense. — Antti Haapala -- Слава Україні, Apr 04 '21 at 08:03
@AnttiHaapala look at this one https://stackoverflow.com/questions/66905392/passing-char-uninitialiezed-pointer-address-and-allocation-and-extraction-of-mem — user786, Apr 04 '21 at 08:10
I did see it, hence my comments there. What I said above does not contradict what Dude said there. Your code does not constitute a [mre], it is just some dump into the question box. It is very easy to tell because you didn't even care to strip out commented code! — Antti Haapala -- Слава Україні, Apr 04 '21 at 08:11
https://stackoverflow.com/questions/66853263/creating-my-own-struct-for-a-packet-that-need-to-be-sent-from-a-server-to-a-clie?noredirect=1#comment118176676_66853263 — wildplasser, Apr 04 '21 at 09:47
@AnttiHaapala what I found is that in `memcpy` the destination is `NULL` but the struct which contains destination is NOT `NULL` so basically `struct packets` is not NULL but gdb `print` printing 0 for `unsigned char h_dest[ETH_ALEN];/*destination eth addr */` — user786, Apr 06 '21 at 08:07
@MartinJames `We cannot see any obvious problems either because you have not supplied the data structs or the calling code. How can we tell if your pay pointer is valid?` please look at the `update 2` of this question and my last comment — user786, Apr 06 '21 at 08:09
@WeatherVane I have updated my question with debug info and `struct packet definition` char *pay` is also defined in calling function. I think that's enough info and two ppl have also voted to down-close the question. Can u please look at it. At least add what else do u need me to do with `gdb` to down-close. — user786, Apr 06 '21 at 08:26
@AnttiHaapala full code of the program. Please look at this. to all as well who asked for code https://github.com/fawadfwd/PacketMMAP_Server — user786, Apr 07 '21 at 10:26
Try running your code through valgrind. If you're mismanaging memory it will tell you where. — dbush, Apr 07 '21 at 12:34
@666 It's a different tool. It tells you if you write outside memory bounds, dereference invalid pointers, or leak memory. Give it a try. — dbush, Apr 07 '21 at 12:37

score 3 · Accepted Answer · answered Apr 10 '21 at 19:14

You report that given this setup ...

    void get_payload_to_send(struct packets *p,char **pay)
    {
        printf("%s --->>> %s",p->ip_source,p->ip_dest);
        int t1;
        *pay=(char *) malloc(sizeof(t1)+1020/*1000=payload*/);
        memset (*pay, 0, sizeof(t1)+1020);
        struct ethhdr *eth = (struct ethhdr *) *pay;
        struct iphdr *ip = (struct iphdr *) (*pay + sizeof(struct ethhdr));
        struct tcphdr *tcp = (struct tcphdr *) (*pay + sizeof(struct ethhdr) + sizeof(struct iphdr));

... you find that this memcpy() throws a segfault:

memcpy(eth->h_dest,p->h_source,sizeof(eth->h_source));

, with gdb revealing to you that eth->h_dest is null at the time of the call.

Well, the destination pointer being null completely explains the segfault. And if eth->h_dest is a pointer and all-bits-zero is a null pointer representation for its type, then of course it is null. You set it to all-bits zero with your memset() call.

You need either to allocate sufficient (additional) space for eth->h_dest to point to before copying data there, or else use pointer assignment to point it to an existing object. The context is not sufficiently clear for me to judge which of those would be more appropriate.

After the post I did exactly this what u said `else use pointer assignment to point it to an existing object` this thing was fixed some time ago. But it was very important for me to this ur info. Thanks for this. Can u please look at this question too https://stackoverflow.com/questions/67042985/whats-the-point-of-dma-proxy-or-can-we-use-kernel-data-structures-in-userspace — user786, Apr 11 '21 at 09:09
can you please tell little about `copying ethernet frames to user buffer` this is the question I asked https://stackoverflow.com/questions/67054910/question-about-dma-proxy-on-how-its-used-to-copy-ethernet-frames-to-user-buffer. btw thank you very much for answering the question in above link. I have totally accepted the answer, thanks again. — user786, Apr 12 '21 at 08:28
If u know any kernel programming please look at the above comment link. Let me know if u need any changes to the question. I kind of stuck for last 2 days just a comment may be. I got no replies still. — user786, Apr 14 '21 at 15:56

Jose · Answer 2 · 2021-04-12T19:58:38.197

2

There are two mayor errors on these lines:

struct ethhdr *eth = (struct ethhdr *) *pay;
struct iphdr *ip = (struct iphdr *) (*pay + sizeof(struct ethhdr));
struct tcphdr *tcp = (struct tcphdr *) (*pay + sizeof(struct ethhdr) + sizeof(struct iphdr));

Memory pointed by pay* was allocated using a magic number, the size of ethhdr, iphdr and tcphdr might be the same, but I wouldn't like to work on that code, accesses out of boundaries are likely.
Regarding alignment, it may happen that ethhdr has to land on an multiple of X, but pay is not. That will trigger an error (malloc will return an address whose alignment is at least max_align_t, but everything may happen with the extra additions).

edited Apr 12 '21 at 19:58

answered Apr 10 '21 at 17:34

Jose

3,306
1
17
22

There is no strict aliasing violation in the lines quoted, nor do they appear to be creating a situation that would facilitate such an issue later. The pointed-to memory has no effective type until it is written to. – John Bollinger Apr 10 '21 at 17:54
@JohnBollinger I have updated my answer to clarify why I do think there is a strict aliasing problem. – Jose Apr 11 '21 at 00:07
Regarding alignment, I think we are saying the same, correct me if I am wrong. – Jose Apr 11 '21 at 00:08
1

Read the SAR in the context of [its preceding paragraph](https://port70.net/~nsz/c/c11/n1570.html#6.5p7), and pay particular attention to the careful use of the term "effective type" in the SAR itself (paragraph 6.5/8). If you still think you see SAR violations, then the explanation in this answer should be augmented with what you think the effective types of the objects involved are, and what the types of the lvalues being used to access them are. – John Bollinger Apr 11 '21 at 12:33
@JohnBollinger you are right, I was 100% sure that memset would change the effective type, but it doesn't. Thanks for your time and the reference. – Jose Apr 12 '21 at 19:58

Glärbo · Answer 3 · 2021-04-07T13:26:09.293

The problem is that you are not supplying the correct pointer to memset():

char *pay=(char *) malloc(sizeof(t1)+1020);
memset (*pay, 0, sizeof(t1)+1020);

clears memory using the first char in the dynamically allocated array pay as the target address.

The correct form is

char *pay = malloc(sizeof(t1)+1020);
memset (pay, 0, sizeof(t1)+1020);

This is because pay is the pointer that refers to the dynamically allocated array, whereas *pay refers to the value of its first element just like pay[0].

(In C, the explicit cast from void *, such as the return value from malloc(), is not needed.)

The same error is duplicated several times:

struct ethhdr *eth = (struct ethhdr *) *pay;
struct iphdr *ip = (struct iphdr *) (*pay + sizeof(struct ethhdr));
struct tcphdr *tcp = (struct tcphdr *) (*pay + sizeof(struct ethhdr) + sizeof(struct iphdr));

These declare and initialize pointers to addresses derived from the first char in the dynamically allocated array pay, rather than addresses relative to the beginning of the array.

Again, their correct form is

struct ethhdr *eth = (struct ethhdr *) pay;
struct iphdr *ip = (struct iphdr *) (pay + sizeof(struct ethhdr));
struct tcphdr *tcp = (struct tcphdr *) (pay + sizeof(struct ethhdr) + sizeof(struct iphdr));

because *pay refers to the value of the first element in the pay array, whereas pay refers to the address of the array.

Apparently, the actual code is

char **pay;

*pay = malloc(sizeof (t1) + 1020);
memset (*pay, 0, sizeof (t1) + 1020);

This lacks two essential checks:

That pay is not NULL
That malloc() succeeded

The code can fail for either reason with the symptoms described. At minimum, rewrite this as


assert(pay != NULL);
*pay = malloc(sizeof (t1) + 1020);
assert(*pay != NULL);
memset(*pay, 0, sizeof (t1) + 1020);

with the eth, ip, and tcp variable definitions kept in their original form (i.e., with *pay).

Now exception is getting thrown at this line `ip->daddr=s2.sin_addr.s_addr;` inside `get_payload_to_send` function in thread two. GDB says `ip` is `0xe` but when I try to print `ip->daddr` gdb says `Cannot access memory at address 0x1e` — user786, Apr 07 '21 at 12:35
@666: You have the same error consistently in your code. You use the value of the first element of the `pay` array, `*pay`, when you should use the address of the array, `pay`. — Glärbo, Apr 07 '21 at 12:50
@Glärbo The line in the function is `*pay=(char *) malloc(sizeof(t1)+1020/*1000=payload*/);`. The function's signature is `void get_payload_to_send(struct packets *p,char **pay)`. So `*pay` is a `char *` that holds the address returned by `malloc`. — dbush, Apr 07 '21 at 13:18

SegFault on memcpy and source and destination are defined and initialized

3 Answers3