Heap corruption when making two bytes into a short. C++

Question

I am developing a program in C++ and encounter this error when I run this code:

short readshort() {
    short val=0;
    (&val)[1]=data.front(); data.pop_front();
    (&val)[0]=data.front(); data.pop_front();
    return val;
}

This is the error I get:

Run-Time Check Failure #2 - Stack around the variable 'val' was corrupted.

I should mention now, that "data" is defined with std::list data;

I think I know what is the problem, but I cannot think of an easy solution. I think this error is cause by the "val" being stored in the stack, and not as a pointer. When I try to access the data pointer by "val" I get this error.

The solution I thought about was allocating "val" like this:

short readshort() {
    short* val=new short;
    val[1]=data.front(); data.pop_front();
    val[0]=data.front(); data.pop_front();
    return *val;
}

But I can't see a way to delete "val" once I have returned it without having to delete it outside the function every time. Is there a way this can be done in C++ wihtout a memory leak? I havn't seen anyone split a variable type (e.g. short) into bytes using "(&val)[1]" before, and wondered if this was because it gave rise to a number of problems, or is it just not a known method?

Coming back to the real question, how can I make these two bytes into short (or large data type)? And is there a better way of doing this than what I have tried?

One last thing, I know that java has an automatic garbage collector that cleans up memory leaks automatically. Does C++ offer the same kind of device? I heard somthing about Smart Pointers, but I don't know what they are ;)

Answer 1

This is safe and simple:

int16_t readshort()
{
    union { int16_t s; char val[2]; } u;
    u.val[1]=data.front(); data.pop_front();
    u.val[0]=data.front(); data.pop_front();
    return *(int16_t*)u.val;
}

Answer 2

You need to cast your pointer to char*.

((char *)(&val))[1]=data.front();
((char *)(&val))[0]=data.front();

I think in your case: (&val)[1]=data.front(); you write data to second short. As result you get error.

Answer 3

(&val)[1]=data.front() an error by doing (&val)[1] you are writing next memory location to val that is not defined.

(&val)[i] means *(&val + i )
(&val)[0] means *(&val + 0 ) = *(&val) this fine
(&val)[1] means *(&val + 1 ) = But this is error because your declaration is short val so we can access only val location.

+----+----+----+---+---+----+----+----+---+----+ 
|val      |        |
+----+----+----+---+---+----+----+----+---+---+----+  
 201   202  203 204 205 206  207   208 209 210  211
  ^           ^ 
   |          |
 &val         (&val + 1) 
              its not defined.   

you could use tyoecase like @Carl Norum suggested. I am just writing second form to do this.

char *ptr = (char*)&val;

ptr[0]=data.front()
ptr[1]=data.front()

But if your need val as short and wanted to access individual bytes. I would like to suggest union:

union Data{
 short val;
 char ch1;
 char ch2;
};

union Data d; 

d.ch1 = data.front()
d.ch2 = data.front()

Answer 4

(&val)[1] accesses memory you don't have allocated. Bam - undefined behaviour. Both of your examples have the same problem.

If you want to split bytes like that, you need to use char pointers to access the individual bytes:

short readshort() {
    short val=0;
    ((char *)&val)[1]=data.front(); data.pop_front();
    ((char *)&val)[0]=data.front(); data.pop_front();
    return val;
}

This code is really pretty ugly, though. Why not just:

short readshort() {
    short val=0;
    val  = data.front() << 8; data.pop_front();
    val |= data.front() << 0; data.pop_front();
    return val;
}

Depending on endianness, you may need to swap the positions of the 8 and 0 there.