aakash-sahai / nanopb

Automatically exported from code.google.com/p/nanopb
zlib License
0 stars 0 forks source link

pb_istream_from_buffer() returns pointer to stack-allocated object #24

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
nanopb works for me, but the following code is suspicious in pb_decode.c:

pb_istream_t pb_istream_from_buffer(uint8_t *buf, size_t bufsize)
{
    pb_istream_t stream;
...
    return stream;
}

since pb_istream is allocated on the stack, referring to it upstream for struct 
assignment of the return value depends on the compiler/runtime support leaving 
the stack intact, which might be a hit-and-miss game

Suggested change:

typedef struct _pb_istream_t *pb_istream_ptr_t;

void pb_istream_from_buffer(pb_istream_ptr_t *stream, uint8_t *buf, size_t 
bufsize)
{
    stream->callback = &buf_read;
    stream->state = buf;
    stream->bytes_left = bufsize;
}

Usage:

pb_istream_t foo_stream;
pb_istream_from_buffer(&foo_stream, buf, size):

Original issue reported on code.google.com by haberl...@gmail.com on 1 Aug 2012 at 7:10

GoogleCodeExporter commented 9 years ago
It doesn't return a pointer, instead, it returns a struct. This results in a 
bit of extra copying, but 12 bytes is not that big a struct.

However, it could be better to change it anyway. Some compilers (e.g. sdcc) 
don't support returning structs.

Original comment by Petteri.Aimonen on 1 Aug 2012 at 7:38

GoogleCodeExporter commented 9 years ago
right, the headline is wrong, but my bitching aint ;)

the issue is: the returned struct is allocated on the stack, and by the time 
it's assigned there's no guarantee the stack is still intact:

see second answer in: 
http://stackoverflow.com/questions/4911288/allocate-struct-from-function-in-c

so the solution is to allocate the memory for the struct in the caller, and 
pass a pointer/reference to the struct so pb_istream_from_buffer can fill in 
the details

Original comment by haberl...@gmail.com on 1 Aug 2012 at 8:10

GoogleCodeExporter commented 9 years ago
But in turn, see the answers in:
http://stackoverflow.com/questions/9653072/return-a-struct-from-a-function-in-c

(Or this answer in your link: "Return a copy of the struct (this is OK for 
small structs)")

It's perfectly valid C to return a struct by-value. The caller makes a copy of 
it on the stack, just like if you returned an integer.

I agree that there would be a problem with "return &stream;". However, "return 
stream;" is ok.

Original comment by Petteri.Aimonen on 1 Aug 2012 at 8:32

GoogleCodeExporter commented 9 years ago
you win ;)

issue closed

Original comment by haberl...@gmail.com on 1 Aug 2012 at 8:35

GoogleCodeExporter commented 9 years ago
:)

Original comment by Petteri.Aimonen on 1 Aug 2012 at 8:39

GoogleCodeExporter commented 9 years ago
Issue 95 has been merged into this issue.

Original comment by Petteri.Aimonen on 12 Dec 2013 at 8:31