-4

I was about to return unsigned long array, from a function called displaypcapStas(): But i was not able to do that, i don't know whats going on: here is my c code:

unsigned long * displaypcapStats()
{
int            nBytes, testId, num_pcaps;
char           buffer[1024];
socklen_t      addr_size;
int            index = 0, i, cmd_id, len, parLen, result;
char           pcapname[256];
int statNum1 = 0;

unsigned long *pcapstats;
unsigned long  *value;

unsigned long  tx_pkts, tx_bytes, rx_pkts, rx_bytes, tx_pkts_op, tx_bytes_op, rx_pkts_op, rx_bytes_op;
int            east_west, west_east, err_pkts;
int            passvalue = 0;

//Create UDP socket    clientSocket = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);

//Configure settings in address struct
bzero((char *) &serverAddr, sizeof(serverAddr));
serverAddr.sin_family = AF_INET;
serverAddr.sin_port = htons(ATICARA_AUTO_PORT);
//inet_aton(host, &serverAddr.sin_addr);
inet_aton(getenv(ATICARA_HOST_IP), &serverAddr.sin_addr);

//Initialize size variable to be used later on
addr_size = sizeof(serverAddr);

buffer[index++] = ((tid>>8)&0xff);
buffer[index++] =  tid & 0xff;
buffer[index++] =  ((DISPLAY_PCAP_STATS >> 8) & 0xff);
buffer[index++] =  DISPLAY_PCAP_STATS & 0xff;

//Send message to server
sendto(clientSocket,buffer,index,0,(struct sockaddr *)&serverAddr,addr_size);

//Receive message from server for num of pcaps
nBytes = recvfrom(clientSocket,buffer,BUFSIZE,0,(struct sockaddr    *)&serverAddr, &addr_size);
printf("Received Pcap Stats successfull:%d\n", nBytes);
tid = ((buffer[0] << 8) & 0xff00) | (buffer[1] & 0xff);
result = ((buffer[2] << 8) & 0xff00) | (buffer[3] & 0xff);

if (result == 1){
    index = 3;
    num_pcaps = ((buffer[++index] << 8) & 0xff00) | (buffer[++index] & 0xff);
    printf("\nNumber of pcaps: %d\n", num_pcaps);

    pcapstats = malloc(8*num_pcaps*sizeof(unsigned long));
for(i = 0; i < num_pcaps; i++) {

        tx_pkts = (((uint64_t)buffer[++index] << 56) & 0xff00000000000000) |
                  (((uint64_t)buffer[++index] << 48) & 0xff000000000000) |
                  (((uint64_t)buffer[++index] << 40) & 0xff0000000000) |
                  (((uint64_t)buffer[++index] << 32) & 0xff00000000) |
                  (((uint64_t)buffer[++index] << 24) & 0xff000000) |
                  (((uint64_t)buffer[++index] << 16) & 0xff0000) |
                  (((uint64_t)buffer[++index] << 8) & 0xff00) |
                  ((uint64_t)buffer[++index] & 0xff);
        printf("%ld ", tx_pkts);

        tx_bytes = ...



        pcapstats[statNum1++] = tx_pkts;
        pcapstats[statNum1++] = tx_bytes;
        pcapstats[statNum1++] = rx_pkts;
        pcapstats[statNum1++] = rx_bytes;
        pcapstats[statNum1++] = tx_pkts_op;
        pcapstats[statNum1++] = tx_bytes_op;
        pcapstats[statNum1++] = rx_pkts_op;
        pcapstats[statNum1++] = rx_bytes_op;

    }
    pcapstats[statNum1++] = '\0';
    printf("Pcap stats Display Successfull:%d\n",nBytes);
    return(pcapstats);
}
else {
    printf("\nPcap stats Display Unsuccessfull!:%d",nBytes);
    *value = buffer[3];
    return value;
 }
}

I am getting segmentation fault, at last name.

1 Answers1

2

You are modifying index twice without an intervening sequence point:

num_pcaps = ((buffer[++index] << 8) & 0xff00) | (buffer[++index] & 0xff);

(Stopped looking when I found this. There may be more bugs.)

Community
  • 1
  • 1
Klas Lindbäck
  • 33,105
  • 5
  • 57
  • 82
  • @ klas Lineback: hi look before num_pcaps = ((buffer[++index] << 8) & 0xff00) | (buffer[++index] & 0xff); i have mentioned index = 3; which means that each time before it takes the index value it will interment and it will take next value. Like 4, 5, .... Which is nothing but the buffer value. I am not modifying index twice, instead I am taking index value after increment. – Adarsha Verma Feb 10 '17 at 19:45
  • @AdarshaVerma You're wrong. If `((buffer[++index] << 8) & 0xff00)` is zero, you will then execute `(buffer[++index] & 0xff)`. That is **two** increments of `index`. – Carey Gregory Feb 10 '17 at 20:20
  • @CareyGregory `|` does not short circuit, this code always has two increment expressions executed (and is therefore undefined behaviour) – M.M Feb 11 '17 at 07:10
  • @M.M You're right, I stand corrected. – Carey Gregory Feb 11 '17 at 15:54
  • @AdarshaVerma Please read [this](http://stackoverflow.com/questions/949433/why-are-these-constructs-using-undefined-behavior) – M.M Feb 13 '17 at 04:53
  • @M.M and CareyGregory I agree with you guys but i am getting correct value for num_pcaps. This is what i am sending from server socket through buffer called buf and it has a value 15: buf[index++] = ((num_pcaps>>8)&0xff); buf[index++] = num_pcaps & 0xff; Same way in client side i am receiving like this: num_pcaps = ((buffer[++index] << 8) & 0xff00) | (buffer[++index] & 0xff); i am getting proper output. And if i am wrong while increment index twice, it shouldn't give proper output right??? – Adarsha Verma Feb 13 '17 at 05:51
  • @AdarshaVerma Follow the link in my answer. Read the answers. And to answer your question regarding undefined behaviour giving correct output please read this: http://stackoverflow.com/questions/2397984/undefined-unspecified-and-implementation-defined-behavior Of course, if your compiler should happen to produce the behaviour you desire then this bug, big as it is, is not the one causing your problems. – Klas Lindbäck Feb 13 '17 at 08:35