is this code safe?

[Songling]

Hi,

Please help to check if the following code is safe.

void* dequeue(MSG_Q_ID q_ptr)
{
U32 msg_ptr = 0;

if (msgQReceive((q_ptr), (char*) &msg_ptr, 4, 0) >=0)
{
return((void*) msg_ptr);
}
return NULL;
}

int main(void)
{
// get qid....

MyType *my_ptr = (MyType *) dequeue( qid);

// process dequeued msg through my_ptr....
}

I thought that after dequeue() returns, msg_ptr is deallocated from the
stack, and
my_ptr is pointing to some invalid memory space. But seems like the code
works
fine. So is this a real problem?

Thanks,

Songling

 

[Richard Tobin]

U32 msg_ptr = 0;

if (msgQReceive((q_ptr), (char*) &msg_ptr, 4, 0) >=0)
{
return((void*) msg_ptr);
}

Bleah. Presumably U32 is a 32-bit integer datatype and msgQReceive
is putting 4 bytes of data into it, which are in fact a pointer. So
this is making lots of assumptions about the implementation.

But supposing it's right for your implementation, you are returning
the *value* of msg_ptr. The fact the the msg_pointer variable is
gone, and that you then interpret the value as a pointer:

MyType *my_ptr = (MyType *) dequeue( qid);

is unimportant. my_ptr will point to what msg_ptr pointed to, not
to msg_ptr.

-- Richard

 

[Karthik Kumar]

Hi,

Please help to check if the following code is safe.

void* dequeue(MSG_Q_ID q_ptr)
{
U32 msg_ptr = 0;

What is U32. Is that unsigned int ?
In that case then of course it is a problem.
You need to look at the compiler that you are using.

I wrote a small problem to look into the problem.

#include <stdlib.h>

int * GetLocalAdd() {
int a = 4;
return &a;
}

int main() {
GetLocalAdd();

return EXIT_SUCCESS;
}


C:\temp>gcc -g -Wall -ansi -pedantic local.c
local.c: In function `GetLocalAdd':
local.c:5: warning: function returns address of local variable

C:\temp>gcc --version
gcc (GCC) 3.3.1 (mingw special 20030804-1)
Copyright (C) 2003 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


Try to set the warning setting for your compiler . And you should be
able to catch these errors.

 

[Gordon Burditt]

Please help to check if the following code is safe.

void* dequeue(MSG_Q_ID q_ptr)
{
U32 msg_ptr = 0;

What's a U32? A spy plane? An Unsigned 32-bit POINTER? You seem
to be using it like a pointer.

if (msgQReceive((q_ptr), (char*) &msg_ptr, 4, 0) >=0)

You're casting a pointer-to-U32 into a pointer-to-char. This is
of questionable validity, especially if U32 is a pointer-to-something.

Is the contents of msg_ptr supposed to be set by msgQReceive?

{
return((void*) msg_ptr);
}
return NULL;
}

int main(void)
{
// get qid....

MyType *my_ptr = (MyType *) dequeue( qid);

// process dequeued msg through my_ptr....
}

I thought that after dequeue() returns, msg_ptr is deallocated from the
stack,

You are returning a *COPY* of msg_ptr from dequeue(). This is not a problem
by itself that msg_ptr goes out of scope.

msg_ptr points at something msgQReceive set it to. Is that still valid when
main() gets hold of it? I don't know. It is likely as valid as when dequeue()
got hold of it, but no way to be sure.

and
my_ptr is pointing to some invalid memory space.

How do you know WHAT my_ptr is pointing at? It's probably NOT pointing at
an auto variable deallocated when dequeue() returns, unless msgQReceive
sets msg_ptr to point at ITSELF.

But seems like the code
works

For what definition of "works"? It looks like a disaster to me.

fine. So is this a real problem?

Please decide what type msg_ptr is supposed to be.

Gordon L. Burditt

 

[red floyd]

What's a U32? A spy plane? An Unsigned 32-bit POINTER? You seem
to be using it like a pointer.

No, it's 16 socially aware Irish rock bands.

 

[Christian Bau]

Hi,

Please help to check if the following code is safe.

void* dequeue(MSG_Q_ID q_ptr)
{
U32 msg_ptr = 0;

if (msgQReceive((q_ptr), (char*) &msg_ptr, 4, 0) >=0)
{
return((void*) msg_ptr);
}
return NULL;
}

int main(void)
{
// get qid....

MyType *my_ptr = (MyType *) dequeue( qid);

// process dequeued msg through my_ptr....
}

The first Athlon64 user who tries to run this code will slap it round
your head. Whoever wrote this should never be allowed anywhere near a C
compiler.

 

[Targeur fou]

Hi,
Hi

Please help to check if the following code is safe.

It seems it is.

void* dequeue(MSG_Q_ID q_ptr)
{
U32 msg_ptr = 0;

if (msgQReceive((q_ptr), (char*) &msg_ptr, 4, 0) >=0)
{
return((void*) msg_ptr);
}
return NULL;
}

It would have been unsafe if you returned the adress of msg_ptr.
Happily, a copy of msg_ptr is done and returned before the deallocation
of msg_ptr from the stack.

int main(void)
{
// get qid....

MyType *my_ptr = (MyType *) dequeue( qid);

// process dequeued msg through my_ptr....
}

I thought that after dequeue() returns, msg_ptr is deallocated from the
stack, and
my_ptr is pointing to some invalid memory space. But seems like the code
works
fine. So is this a real problem?

It's not a real problem if it works with your implementation and that
the behavior of your several pointer <-> integer conversions is well
defined by your implementation. But it's not portable. Note that C99
provides two integer types capable to hold pointer values (intptr_t and
uintptr_, stdint.h).

Regis

 

[Targeur fou]

Hi,



What is U32. Is that unsigned int ?
In that case then of course it is a problem.
You need to look at the compiler that you are using.

Why it would be a problem? That's not the adress of msg_ptr which is
returned, it's msg_ptr itself.

I wrote a small problem to look into the problem.

#include <stdlib.h>

int * GetLocalAdd() {
int a = 4;
return &a;
}

No, that's not the OP did. It's something like that :

int* GetAnAdress()
{
int a = 4;
return (int*)a;
}

What you get is a pointer to an int, an address, whose the value may be
4 or something else (depending on the conversion int -> int* int the
implementation).

Regis

 

[Targeur fou]

The first Athlon64 user who tries to run this code will slap it round
your head. Whoever wrote this should never be allowed anywhere near a C
compiler.

Perhaps with Itanium too.
Try with U64

Regis

 

[Songling]

I also think the code is safe now, assuming that we're using 32-bit
architecture.
Nothing fancy about the code, just use message queue to pass msg pointers.
The actual memory is allocated somewhere else.

I read about the 64-bit architecture from Richard Stevens' book long time
ago,
but never had a chance to use it.

Thanks for all the help!

Songling

 

[Jack Klein]

Hi,

Please help to check if the following code is safe.

No, the code is hideously unportable and hideously unsafe.

void* dequeue(MSG_Q_ID q_ptr)
{
U32 msg_ptr = 0;

Assuming you mean unsigned long for U32, this is completely unsafe.
If you want a pointer to void, why not define a pointer to void?

void *msg_ptr = NULL;

if (msgQReceive((q_ptr), (char*) &msg_ptr, 4, 0) >=0)
{

On the compiler I was using at work today, sizeof(unsigned long) is 32
bits.

return((void*) msg_ptr);

No guarantees at all on casting a unsigned long to a pointer to void.

}
return NULL;
}

int main(void)
{
// get qid....

MyType *my_ptr = (MyType *) dequeue( qid);

No guarantees at all on casting a pointer

// process dequeued msg through my_ptr....
}

I thought that after dequeue() returns, msg_ptr is deallocated from the
stack, and
my_ptr is pointing to some invalid memory space. But seems like the code
works
fine. So is this a real problem?

Thanks,

Songling

Yes, this is a real problem. This is exactly the sort of careless,
crappy code written but undisciplined amateurs and idiots that has
brought the current state of software development to the sorry depths
it currently occupies.

Nobody who worked with me would ever write code like this a third
time. After a detailed explanation and warning the first time, they
would be looking for a new job after the second time.

 

[buda]

This is exactly the sort of careless,
crappy code written but undisciplined amateurs and idiots that has
brought the current state of software development to the sorry depths
it currently occupies.

I understand your anger, but still think it is uncalled for to use such
harsh words. I expect that you didn't mean to say that the OP is an idiot,
but one could easily interpret that sentence the wrong way. This sort of
response only makes people hesitant about asking for the advice of more
experienced programmers, and that in turn does nothing to improve the
current (or future) state of software development. The rest of the reply was
just what was expected from the OP I suppose.

 

[Christopher Benson-Manica]

I understand your anger, but still think it is uncalled for to use such
harsh words.

I disagree; the times that Jack has called my code "idiotic" have been
completely justified. Stupid code is stupid code, and sugarcoating
that fact in the name of preserving self esteem isn't going to change
it. If you think this is hard on the OP, getting fired would be
substantially worse.

 

[Richard Tobin]

I disagree; the times that Jack has called my code "idiotic" have been
completely justified.

The suggestion was that the OP was idiotic, not just his code. I
don't think you can reasonable infer that he is.

It's pretty clear that the code is derived from an example or manual
for some library, rather than being the OP's own work. It may well be
what the library writers intend you to do.

-- Richard

 

[buda]

buda <[email protected]> spoke thus:

Stupid code is stupid code, and sugarcoating
that fact in the name of preserving self esteem isn't going to change
it. If you think this is hard on the OP, getting fired would be
substantially worse.

Surely you realize that not all the people here are professional programmers
(not sure about the OP, but considering the code I'd say he's not). In fact,
I'm guessing most of the people who ask questions aren't; they're either
people who are in the process of becoming a professional programmer, or
people who do programming "for fun" in their spare time. Stomping on them on
a personal level seems unjustified. You can say that some piece of code is
useless, or even idiotic, but to suggest that the person who wrote it is an
idiot (most likely because he doesn't have more than a year of C
experience... if that) is definitely not the best approach. Don't get me
wrong... I think the comments were exactly what the OP wanted to get, but
I'm sure he didn't feel too good being called an idiot. There are actually
real people in front of the computer reading this stuff... sometimes it's a
good idea to think about that before writing something that can surely be
offensive.

 

[Richard Bos]

I understand your anger, but still think it is uncalled for to use such
harsh words. I expect that you didn't mean to say that the OP is an idiot,

Probably not, but I dare say that if he does not fall in the category
"undisciplined amateur", then he probably belongs to "not yet
disciplined student taught by an undisciplined amateur or idiot".

Richard