is this code safe?

Discussion in 'C Programming' started by Songling, Sep 8, 2004.

  1. Songling

    Songling Guest

    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
    Songling, Sep 8, 2004
    #1
    1. Advertising

  2. In article <cho0kg$46l$>,
    Songling <> wrote:

    > 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
    Richard Tobin, Sep 8, 2004
    #2
    1. Advertising

  3. Songling wrote:

    > 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.




    --
    Karthik.
    Karthik Kumar, Sep 9, 2004
    #3
  4. >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
    Gordon Burditt, Sep 9, 2004
    #4
  5. Songling

    red floyd Guest

    Gordon Burditt wrote:

    >
    > 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.
    red floyd, Sep 9, 2004
    #5
  6. In article <cho0kg$46l$>,
    "Songling" <> wrote:

    > 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.
    Christian Bau, Sep 9, 2004
    #6
  7. Songling

    Targeur fou Guest

    Songling wrote:

    > 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, Sep 9, 2004
    #7
  8. Songling

    Targeur fou Guest

    Karthik Kumar wrote:

    > Songling wrote:
    >
    >> Hi,


    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.


    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, Sep 9, 2004
    #8
  9. Songling

    Targeur fou Guest

    Christian Bau wrote:

    > In article <cho0kg$46l$>,
    > "Songling" <> wrote:
    >
    >
    >>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.


    Perhaps with Itanium too.
    Try with U64 ;)

    Regis
    Targeur fou, Sep 9, 2004
    #9
  10. Songling

    Songling Guest

    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

    "Targeur fou" <> wrote in message
    news:cho6lk$fpr$...
    >
    >
    > Songling wrote:
    >
    > > 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
    >
    Songling, Sep 9, 2004
    #10
  11. Songling

    Jack Klein Guest

    On Wed, 8 Sep 2004 18:21:03 -0400, "Songling"
    <> wrote in comp.lang.c:

    > 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.

    --
    Jack Klein
    Home: http://JK-Technology.Com
    FAQs for
    comp.lang.c http://www.eskimo.com/~scs/C-faq/top.html
    comp.lang.c++ http://www.parashift.com/c -faq-lite/
    alt.comp.lang.learn.c-c++
    http://www.contrib.andrew.cmu.edu/~ajo/docs/FAQ-acllc.html
    Jack Klein, Sep 9, 2004
    #11
  12. Songling

    buda Guest

    "Jack Klein" <> wrote in message
    news:...
    > 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.
    buda, Sep 9, 2004
    #12
  13. buda <> spoke thus:

    > 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.

    --
    Christopher Benson-Manica | I *should* know what I'm talking about - if I
    ataru(at)cyberspace.org | don't, I need to know. Flames welcome.
    Christopher Benson-Manica, Sep 9, 2004
    #13
  14. In article <chq5ht$mnk$>,
    Christopher Benson-Manica <> wrote:

    >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
    Richard Tobin, Sep 9, 2004
    #14
  15. Songling

    buda Guest

    "Christopher Benson-Manica" <> wrote in message
    news:chq5ht$mnk$...
    > buda <> 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.
    buda, Sep 10, 2004
    #15
  16. Songling

    Richard Bos Guest

    "buda" <> wrote:

    > "Jack Klein" <> wrote in message
    > news:...
    > > 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,


    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
    Richard Bos, Sep 10, 2004
    #16
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Gabriel Rossetti
    Replies:
    0
    Views:
    1,294
    Gabriel Rossetti
    Aug 29, 2008
  2. Replies:
    1
    Views:
    326
    Brian Candler
    Aug 12, 2003
  3. Aredridel

    Not just $SAFE, but damn $SAFE

    Aredridel, Sep 2, 2004, in forum: Ruby
    Replies:
    19
    Views:
    226
  4. Farrel Lifson

    $SAFE =4 safe enough?

    Farrel Lifson, Aug 29, 2006, in forum: Ruby
    Replies:
    7
    Views:
    95
    Eric Hodel
    Aug 31, 2006
  5. John Nagle
    Replies:
    5
    Views:
    451
    John Nagle
    Mar 12, 2012
Loading...

Share This Page