How to fix "free(): invalid next size (normal) | Canceled (memory dump written)" in C

Question

I am writing a program which actually send raw Packets and Sniffing the Network. I have the Program to build, to send the Packets and to Sniff the Network. Each of them work (mostly) great. If i try to implement a main function which calls first the Function to build and send the Packet and after that the function which sniff, I get a free(): Invalid next size (normal) | Cancelled (memory dump written) Error.

I tried to find the Problem by my own. I called the startSniffer() - Function in different places. Actually in send_tcp_packet befor building the tcp Packet (build_tcp_packet), and after that too. If I call startSniffer() Function befor build_tcp_packet it works. So I call the startSniffer() - Function in build_tcp_packet befor memcpy(...) and it works. So I assume that the error is because of memcpy(...). I removed memcpy(...) and the Programm works well. But actually the Programm needs memcpy(...) to set the Payload in TCP-Header.

So did you have any Idea and helps for my Code???

void startBuilding() {

    unsigned char *data;

    unsigned int packet_size;
    unsigned int data_size;

    src_addr.sin_family = AF_INET;
    inet_aton(srcHost, &src_addr.sin_addr);

    dst_addr.sin_family = AF_INET;
    inet_aton(destHost, &dst_addr.sin_addr);

    data = (char*) malloc(strlen(sendingData) + 1);

    strcpy((char *) data, sendingData);

    data_size = strlen(sendingData);

    switch (ipProto) {
    case IPPROTO_TCP:

        printf("[+] Send TCP packet...\n");

        src_addr.sin_port = htons(tcpSourcePort);
        dst_addr.sin_port = htons(tcpDestPort);

        if (open_RawSocket(ipProto)) {
            send_tcp_packet(raw_socket, src_addr, dst_addr, data, data_size);

        }

}



unsigned int build_tcp_packet(struct sockaddr_in src_addr,
    struct sockaddr_in dst_addr, unsigned char *tcp_packet,
    unsigned char *data, unsigned int data_size) {

    printf("[+] Build TCP packet...\n\n");

    unsigned int length;
    struct tcpheader *tcp;
    length = TCPHDRSIZE + data_size;
    tcp = (struct tcpheader *) tcp_packet;

    tcp->th_sport = src_addr.sin_port;
    tcp->th_dport = dst_addr.sin_port;

    tcp->th_seq = htonl(tcpSeqNum);
    tcp->th_acknum = tcpAcknum;

    tcp->th_reserved = tcpReserved;
    tcp->th_off = tcpOffSet;

    tcp->th_fin = tcpFinFlag;
    tcp->th_syn = tcpSynFlag;
    tcp->th_rst = tcpRstFlag;
    tcp->th_psh = tcpPshFlag;
    tcp->th_ack = tcpAckFlag;
    tcp->th_urg = tcpUrgFlag;
    tcp->th_cwr = tcpCwrFlag;
    tcp->th_ece = tcpEceFlag;

    tcp->th_win = htons(tcpWin);

    tcp->th_urp = tcpUrp;

    tcp->th_sum = tcpSum;

//      startSniffer(); // If i start startSniffer here, it works fine.
    memcpy(tcp_packet + TCPHDRSIZE, data, data_size);
//      startSniffer(); //If i start sartSniffer here, it did not work, and i get the error. 

    return length;
 }


unsigned int build_ip_packet(struct in_addr src_addr, struct in_addr dst_addr,
    uint8_t protocol, unsigned char *ip_packet, unsigned char *data,
    unsigned int data_size) {

    printf("[+] Build IP packet...\n\n");

    struct ipheader *ip;
    ip = (struct ipheader*) ip_packet;

    ip->ip_v = ipv;
    ip->ip_hl = iphl;

    ip->ip_tos = iptype;
    ip->ip_id = htons(ipId);

    if (calculateIpLenInBuildPacket) {
        ip->ip_len = htons(iphl * 4 + data_size);
    } else {
        ip->ip_len = htons(ipLen);
    }

    ip->ip_off = ipOffset;

    ip->ip_moreFrag = ipMoreFragmentFlag;
    ip->ip_doNotFrag = ipDoNotFragmentFlag;
    ip->ip_reserved = ipReserved;
    ip->ip_frag_offset1 = ipFragOffset;

    ip->ip_ttl = ipTTL;
    ip->ip_p = ipProto;

    ip->ip_sum = ipSum;
    ip->iph_sourceip = src_addr.s_addr;
    ip->iph_destip = dst_addr.s_addr;

    return sizeof(struct ipheader) + data_size;
}


int main(int argc, char **argv) {

    startBuilding();

    startSniffer();

}

void send_tcp_packet(int raw_sock, struct sockaddr_in src_addr,
    struct sockaddr_in dst_addr, uint8_t *data, unsigned int data_size) {
    unsigned int packet_size;
    unsigned int ip_payload_size;
    unsigned char *packet;


    packet = (char*) malloc(strlen(data) + 1);
    memset(packet, 0, ETH_DATA_LEN);



    ip_payload_size = build_tcp_packet(src_addr, dst_addr,
        packet + sizeof(struct ipheader), data, data_size);

    packet_size = build_ip_packet(src_addr.sin_addr, dst_addr.sin_addr,
    IPPROTO_TCP, packet, packet + sizeof(struct ipheader), ip_payload_size);

    setOption_RawSocket(raw_sock);

    if (sendto(raw_sock, packet, packet_size, 0, (struct sockaddr *) &dst_addr,
        sizeof(dst_addr)) < 0) {
        perror("sendto");

        exit(1);
    } else {

        printf("Send! \n\n");


    }
    close(raw_sock);

}

Please try to make a more minimal [mcve]. It should be possible to reduce this to a couple of misuses of malloc and free. — Yunnosch, Feb 11 '19 at 16:41
Your problem likely arises from corruption of the memory-management metadata. Likely causes include overrunning dynamically-allocated buffers, double-freeing allocated memory, and continuing to use a pointer after its target has been freed. Very likely this happens some time before the program actually crashes. If you can't spot the problem through code analysis, then engaging a memory-use debugger such as Valgrind would be a good option. — John Bollinger, Feb 11 '19 at 16:43
You need to debug this. Probably there is a buffer overflow somewhere in your code. I suggest you have a close look at the `memcpy` you mention and check that you dont copy more bytes than allowed. — Jabberwocky, Feb 11 '19 at 16:44
https://github.com/google/sanitizers/wiki/AddressSanitizer is very useful for debugging problems like this. It is part of GCC and easy to use. — MarkReedZ, Feb 11 '19 at 16:44
@JohnBollinger Is it normal, that the code works fine, if i did not call `startSniffer()` ? This i confusing. — Sebastian, Feb 11 '19 at 16:51
ASan is great. Just a compile flag and it points you right to the memory problem. — Craftables, Feb 11 '19 at 16:53
@Sebastian, somewhere you have produced *undefined behavior* (for there is no defined behavior that would produce the outcome you describe). There is no applicable sense of normalcy accompanying UB. That you see this particular misbehavior only if you call `startSniffer()` *could* be a sign that the problem is contained within that function, but you cannot be certain of that. — John Bollinger, Feb 11 '19 at 16:55
Note that casing the return from `malloc` is not [best practice](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) in C. If you are using C++ compilation where the cast would be necessary, then better to use `new`. — Clifford, Feb 11 '19 at 17:03

Clifford · Answer 1 · 2019-02-11T17:33:50.017

2

This is dangerous:

packet = (char*) malloc(strlen(data) + 1);
memset(packet, 0, ETH_DATA_LEN);

It will corrupt the heap whenever ETH_DATA_LEN is larger than strlen(data) + 1, the heap corruption will not be immediately detected but may cause subsequent allocations or deallocations (free) to fail.

Suggest:

packet = calloc( ETH_DATA_LEN, 1 ) ;

You made a rash assumption that data is a null terminated string and should have used data_size to determine the length of data in any event, but since packet size is necessarily larger then just the data payload, both are incorrect.

You fail to free packet in this case, so also have a memory leak. Since ETH_DATA_LEN is a constant, there is no need to dynamically allocate in any event. Instead you might have:

char* packet[ETH_DATA_LEN] = {0} ;

edited Feb 11 '19 at 17:33

answered Feb 11 '19 at 16:59

Clifford

88,407
13
85
165

1

@Sebastian : Do what you like. `calloc()` is not essential to the solution here, but if you are allocating then immediately memsetting to zero, it is safer since you then do not need to ensure your allocation and you memset are the same size - which was the error in this case. – Clifford Feb 11 '19 at 17:19
Note that given the number of dangerous practices in just this one function, I would not suggest that I have spotted them all, and have not even looked at the other functions. – Clifford Feb 11 '19 at 17:36
Actually i needed a dynamically allocate in any event. In order that the packet will sendet correctly. Is there a other method to create a dynamical variable, without a memory leak?.... – Sebastian Feb 11 '19 at 18:39
got it. I initialize my packet one step befor i send the packet :))) if you wish, i could share the solution – Sebastian Feb 11 '19 at 19:09

How to fix "free(): invalid next size (normal) | Canceled (memory dump written)" in C

1 Answers1