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

Answer 1

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).

Answer 2

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.

Answer 3

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:

1. That pay is not NULL

2. 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).