Segmentation fault error. I am trying to parse the Payload and extract the value and return the value. What mistake i am doing?

Question

The msg contains a payload, in the form |tag|length|value| size of tag and length is of 2 bytes. size of value is described inside length, and the data type can be uint16, uint32 or uint8.

typedef struct
{
  response_uint16_t msg[5];
}payload;

void func(response_uint16_t *msg, int type)  
{
  int *posPtr = NULL;
  void *temp;
  int valueLen = msg->length;
  posPtr = &msg->tag;
  posPtr += sizeof(msg->tag);
  posPtr += sizeof(msg->length);
  posPtr++;                         //ideally this should now point to value.
  if(type == 1)
  {
    memcpy((U8*)temp, *posPtr, valueLen);
    return(*temp);
  }
  if(type == 2)
  { 
    memcpy((U16*)temp, *posPtr, valueLen);
    return(*temp);
  }
}

int main()
{
  payload *P;
  p = (struct payload*)malloc(sizeof(p));
  int res;
  /*assumne the msg is filled here and value expected to be filled here is of type U16 */
  res =(U16)func(&p->msg[1], 2); 
  res =(U8)func(&p->msg[2], 1);
.
.

}

Can you first update your code to compile without warning, it will help you discover the problems — jo_, Jan 02 '20 at 08:47
`void * memcpy(void *to, const void *from, size_t numBytes);`. You're copying *to* `temp`, but you never assign `temp` any storage. Undefined behavior. — Tibrogargan, Jan 02 '20 at 08:48
if i take void temp; .... memcpy((U16)temp, *posPtr, valueLen); then it throws an error variable or filed is void here!! i need to assign a storage type based on the type, if it is 1 then it should be uint 8 so on — MAMTHA SRI, Jan 02 '20 at 09:12
please provide [How to create a Minimal, Reproducible Example](https://stackoverflow.com/help/minimal-reproducible-example). Code has multiple compilation warnings and errors. `void func(response_uint16_t *msg, int type) ` function returning values?? — TruthSeeker, Jan 02 '20 at 09:17
`posPtr += sizeof(msg->tag);` is incrementing the pointer by `msg->tag` multipled by `sizeof(uint16_t)` bytes. What exactly is the packet structure? — KamilCuk, Jan 02 '20 at 09:24
posPtr = &msg->tag //pointing to x001 tag posPtr += sizeof(msg->tag) //pointing to x002 length posPtr +=sizeof(msg->length) //pointing to x004 value posPtr++; // posPtr should point to the value Since i dont know the how big the the value is, With the help of valueLen i need to read till that much and store it in temo and return the same. — MAMTHA SRI, Jan 02 '20 at 09:31
`posPtr` is `int*`. You are incrementing by `sizeof(int)` multiplied by the value. I am sorry, in my last comment I assumed it's `uint16_t*`. Please read about pointer arithmetic in C. Please edit your question to specify additional information. Please post the `response_uint16_t` type definition. Please post a compilable [MCVE] others can test. Please specify resulting value you want to get. Please post `U16` and `U8` types definitions. Note, `memcpy(..., *posPtr, ...)` is copying from the derefenced pointer value, I doubt there is a valid address there. — KamilCuk, Jan 02 '20 at 09:37
regarding: `memcpy((U8*)temp, *posPtr, valueLen);` and similar statements. There is no need to cast the parameters AND the function is expecting a `void*` for the first parameter — user3629249, Jan 03 '20 at 16:18
the posted code does not compile! Please post a [mcve] so we can reproduce the problem and help you debug it — user3629249, Jan 03 '20 at 16:22

Yingyu YOU · Answer 1 · 2020-01-02T10:06:05.907

There are several issues inside this code snippet:

As @Tibrogargan commented, temp pointer is accessed (copied-to) before being initialized;
Pointer posPtr is declared as int *, but it seems to be advanced by bytes.

  posPtr = &msg->tag;            // <== pointing to `tag`
  posPtr += sizeof(msg->tag);    // <== pointing to element after `tag`, i.e. `length`? NO! Advanced for two integers width since size of (msg->tag) should be 2 as you said
  posPtr += sizeof(msg->length); // <== pointing to element after `length`, i.e. `value`? NO! Same reason

Assuming your platform integer is 32-bit wide. And you said "size of tag and length is of 2 bytes". So posPtr += sizeof(msg->tag) is in fact posPtr += 2. Same case for posPtr += sizeof(msg->length).

initial posPtr --> +----------------------------+
                   | tag    | __two_bytes_pad__ |
      posPtr+1 --> +----------------------------+
                   | length | __two_bytes_pad__ |
      posPtr+2 --> +----------------------------+
                   | value                      |
                   +----------------------------+

Again, even without above issue, the comment in following line looks false. posPtr looks should pointing to next element after value. Anyhow, without knowing your actual data structure, I cannot say exactly where posPtr is pointing to. But this line worth double checking.

posPtr++;                         //ideally this should now point to value.

You are assuming the struct is packed but which is normally not, if not specified when define the struct, see Structure padding and packing for more details.
Pointer temp is declared as void * therefore cannot be directly de-referenced.
Return type of func is void therefore cannot return any value.
You might be assuming the endianness when trying to access the value.

H S · Answer 2 · 2020-01-02T09:25:29.557

1

You need to allocate memory for variable temp using malloc or calloc like this:

  int valueLen = msg->length;
  size_t valueSize = type ==1 ? sizeof(U8) : sizeof(U16);
  void *temp = malloc(valueLen * valueSize);

Then perform memcopy so as to avoid Undefined behaviour caused by copying something to temp variable, which often but not always results in Seg Fault as pointed by @Tibrogargan

edited Jan 02 '20 at 09:25

answered Jan 02 '20 at 09:16

H S

1,211
6
11

Segmentation fault error. I am trying to parse the Payload and extract the value and return the value. What mistake i am doing?

2 Answers2