Page 1 of 2 12 Last
  • Jump to page:
    #1
  1. Banned ;)
    Devshed Supreme Being (6500+ posts)

    Join Date
    Nov 2001
    Location
    Woodland Hills, Los Angeles County, California, USA
    Posts
    9,643
    Rep Power
    4248

    Scorpy's Puzzle of the Week 2007-07-18


    Here's a bit of code
    Code:
    typedef struct {
        /* .... bunch of struct elements here ... */
    } MyStruct;
    
    MyStruct *ptr;
    size_t     i, some_size;
    
    /* set the value of some_size here */
    
    /* allocate an array of structs of length some_size*/
    ptr = malloc(some_size * sizeof(MyStruct));
    if (ptr == NULL) {
        fprintf(stderr, "Could not allocate %zu structs\n", some_size);
        exit(1);
    }
    
    for (i = 0; i < some_size; i++) {
        /* Set the values of array elements (ptr[i]) here */
        ptr[i].elem1 = i;
        snprintf(ptr[i].elem2, sizeof(ptr->elem2), "....");
        ....
    }
    Your goal is to
    (a) point out what could sometimes go wrong with this code.
    (b) how would you fix it.

    Comments on this post

    • gimp agrees : This is a pretty good idea.
    Last edited by Scorpions4ever; July 19th, 2007 at 01:08 PM.
    Up the Irons
    What Would Jimi Do? Smash amps. Burn guitar. Take the groupies home.
    "Death Before Dishonour, my Friends!!" - Bruce D ickinson, Iron Maiden Aug 20, 2005 @ OzzFest
    Down with Sharon Osbourne

    "I wouldn't hire a butcher to fix my car. I also wouldn't hire a marketing firm to build my website." - Nilpo
  2. #2
  3. Devshed God 1st Plane (5500 - 5999 posts)

    Join Date
    Jun 2005
    Posts
    5,964
    Rep Power
    4852
    The information's is a little scanty, Scorp, but that's probably part of the deal. Pointer [i] is the result of pointer arithmetic, which depends (for its validity) upon the size of the elements represented by i. The layout of the struct might or might not correspond to those boundaries, because of padding. Unless one's compiler allows control over the padding, one should refer to members directly, and not relationally.

    Even if one's compiler can be coerced to conform, one should realize that one has shot portability directly in the ***.
  4. #3
  5. Contributed User
    Devshed Specialist (4000 - 4499 posts)

    Join Date
    Jun 2005
    Posts
    4,413
    Rep Power
    1871
    > one should refer to members directly, and not relationally.
    Where was any relational access?
    Everything looked like it was being accessed by member names.

    Back to the original problem.

    > ptr = malloc(some_size * sizeof(MyStruct));
    I prefer the idiom ptr = malloc ( some_size * sizeof *ptr ); as it removes any explicit mention of a type name from the statement, and makes the future maintainer's life easier.

    > "Could not allocate %u structs\n", some_size);
    %u is not necessarily capable of displaying a size_t in all it's glory. If size_t is larger than int, then there is more data on the stack than the %u will extract, and any following format and parameter will be mis-aligned.
    C99 introduces "%zu" to print a size_t portably.
    In C89, "Could not allocate %lu structs\n", (unsigned long)some_size); would at least confine any truncation to the immediate value being printed.

    > strcpy(ptr[i].elem2, "....");
    Without seeing how elem2 was declared, this has all sorts of allocation and overrun woes.
    I'd probably go with some kind of strncpy() which also ensured that a \0 was appended.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper
  6. #4
  7. Super User
    Devshed Novice (500 - 999 posts)

    Join Date
    Sep 2004
    Posts
    648
    Rep Power
    76
    Since the more 'correct' answers have already been given, I'll give an alternate response: I'd eliminate the dynamic allocation and use a fixed size buffer. Of course, given what little information we have from the puzzle, this may not be practical and/or possible depending upon other requirements of the code. However, ditching dynamic allocation for a fixed size buffer gives you the following:

    Advantages:
    +Avoids any potential memory leaks, double frees, etc
    +Simplifies the code, less error checking, etc
    +Possibly faster execution time and/or less memory fragmentation

    Disadvantages:
    -Less flexible
    -Potentially slower execution time or (more likely) uses excess memory
    -Easier to introduce buffer overflows.
  8. #5
  9. ASP.Net MVP
    Devshed Specialist (4000 - 4499 posts)

    Join Date
    Aug 2003
    Location
    WI
    Posts
    4,378
    Rep Power
    1511
    Puzzle of the week? More like year scorpy :p

    The big thing I noticed the is that the memory obtained from malloc is never freed.
    Primary Forum: .Net Development
    Holy cow, I'm now an ASP.Net MVP!

    [Moving to ASP.Net] | [.Net Dos and Don't for VB6 Programmers]

    http://twitter.com/jcoehoorn
  10. #6
  11. Banned ;)
    Devshed Supreme Being (6500+ posts)

    Join Date
    Nov 2001
    Location
    Woodland Hills, Los Angeles County, California, USA
    Posts
    9,643
    Rep Power
    4248
    Let me make a few things clearer (plus one big hint at the end of this post):

    1. Referencing ptr[i] should work fine because otherwise this wouldn't work either:
    Code:
    int *i_array;
    int i;
    /* Allocate 1000 integers */
    i_array = malloc(1000 * sizeof(int));
    for (i = 0; i < 1000; i++)
       i_array[i] = i;
    The same deal will apply if you malloc an array of MyStruct. ptr[i] is the same as *(ptr + i), so it should work fine.

    2. Good call about %zu (hehe, I didn't know about that one), but that's not the issue I'm looking for here. If that section is triggered, the program will quit right afterwards, so not much harm will be done outside a slightly spurious message.

    3. Re: the issue of strcpy(ptr[i].elem2, "...."), assume that it is called with no buffer overflow. If you prefer, that statement could be:
    strlcpy(ptr[i].elem2, sizeof(ptr->elem2), ".....");
    or
    snprintf(ptr[i].elem2, sizeof(ptr->elem2), "....");
    I'll edit the original post and clear that one up.

    4. Not free() ing the malloc() - Well, this is a code snippet, not complete code :). Assume that I'll free() it later down the line.

    With that said, keep trying folks :).

    BIG HINT: C++ programmers may be immune to this issue.
    Last edited by Scorpions4ever; July 19th, 2007 at 01:10 PM.
    Up the Irons
    What Would Jimi Do? Smash amps. Burn guitar. Take the groupies home.
    "Death Before Dishonour, my Friends!!" - Bruce D ickinson, Iron Maiden Aug 20, 2005 @ OzzFest
    Down with Sharon Osbourne

    "I wouldn't hire a butcher to fix my car. I also wouldn't hire a marketing firm to build my website." - Nilpo
  12. #7
  13. No Profile Picture
    Contributing User
    Devshed Regular (2000 - 2499 posts)

    Join Date
    Jul 2006
    Posts
    2,270
    Rep Power
    1737
    Don't tell me this would have anything to do with NULL, would it?
    When you ask a question, be prepared to tell us: what have you tried? If you think you don't need to try anything, we will never be interested in helping you. If you agree with the link, and you refuse to answer that question, you are being a hypocrite.

    Need help with broken code? Your question should be like a good bug report: (1) It has the smallest number of steps to reproduce the problem you see (2) It tells us precisely what you expected to see and (3) It tells us what you saw and how it differed from what you expected. We need all three to help you.
    Want better answers? Tell us what you Googled for and what steps you took to answer your own question.
  14. #8
  15. No Profile Picture
    Contributing User
    Devshed Novice (500 - 999 posts)

    Join Date
    Jan 2004
    Location
    Constant Limbo
    Posts
    989
    Rep Power
    363
    if for some reason you end up with some_size == 0 you cant be sure the return is a valid pointer.
    Originally Posted by man page
    If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned.
    So you may get a unique pointer to nothing and end up trying to access it (at least thats how I read it). In either case you arent guaranteed to get NULL which is what you are comparing against.

    [edit]...well, you wouldnt try to access it in the snippet you have as the loop is controlled by some_size, but you get the point...[/edit]
    True happiness is not getting what you want, it's wanting what you've already got.

    My Blog
  16. #9
  17. No Profile Picture
    Closet coder
    Devshed Beginner (1000 - 1499 posts)

    Join Date
    Feb 2005
    Location
    Plantation, FL <---south florida
    Posts
    1,431
    Rep Power
    153
    could an issue arise from malloc returning a void* and not being explicitly casted to a MyStruct*?
  18. #10
  19. Banned ;)
    Devshed Supreme Being (6500+ posts)

    Join Date
    Nov 2001
    Location
    Woodland Hills, Los Angeles County, California, USA
    Posts
    9,643
    Rep Power
    4248
    Nope, nope and nope, though I'd like to clarify that you can assume some_size is > 0
    Up the Irons
    What Would Jimi Do? Smash amps. Burn guitar. Take the groupies home.
    "Death Before Dishonour, my Friends!!" - Bruce D ickinson, Iron Maiden Aug 20, 2005 @ OzzFest
    Down with Sharon Osbourne

    "I wouldn't hire a butcher to fix my car. I also wouldn't hire a marketing firm to build my website." - Nilpo
  20. #11
  21. ASP.Net MVP
    Devshed Specialist (4000 - 4499 posts)

    Join Date
    Aug 2003
    Location
    WI
    Posts
    4,378
    Rep Power
    1511
    I was thinking about NULL thing, as well, but being a C++ guy I couldn't come up with a reason why it would cause a problem, unless malloc could return something other than NULL or a valid pointer when some_size is greater than 0. Having never used malloc (I've always been able to use new/delete), I haven't had a reason to find out :)

    @natty- I don't think that's it, because ptr is the correct type. Once the assignment is made everything should be okay, and I'm pretty sure you can still implicitly cast a void pointer in C.
    Primary Forum: .Net Development
    Holy cow, I'm now an ASP.Net MVP!

    [Moving to ASP.Net] | [.Net Dos and Don't for VB6 Programmers]

    http://twitter.com/jcoehoorn
  22. #12
  23. No Profile Picture
    Contributing User
    Devshed Regular (2000 - 2499 posts)

    Join Date
    Jul 2006
    Posts
    2,270
    Rep Power
    1737
    Unless NULL were to be defined to something else. It's (void*)0, but were it changed...

    Because in C++, NULL is just 0, so comparison with NULL is pointless.
    When you ask a question, be prepared to tell us: what have you tried? If you think you don't need to try anything, we will never be interested in helping you. If you agree with the link, and you refuse to answer that question, you are being a hypocrite.

    Need help with broken code? Your question should be like a good bug report: (1) It has the smallest number of steps to reproduce the problem you see (2) It tells us precisely what you expected to see and (3) It tells us what you saw and how it differed from what you expected. We need all three to help you.
    Want better answers? Tell us what you Googled for and what steps you took to answer your own question.
  24. #13
  25. Contributed User
    Devshed Specialist (4000 - 4499 posts)

    Join Date
    Jun 2005
    Posts
    4,413
    Rep Power
    1871
    It wouldn't matter if some_size were 0 anyway, because the for loop would never execute, thus dereferencing the pointer returned by malloc (NULL or otherwise) simply won't happen.

    Perhaps it's related to your muse but that would make it an OS issue, not a C vs C++ issue.

    On the off-chance you're thinking about various operating systems use of over-commit whereby requests for huge amounts of memory doesn't immediately guarantee you'll be able to access it all later on.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper
  26. #14
  27. Banned ;)
    Devshed Supreme Being (6500+ posts)

    Join Date
    Nov 2001
    Location
    Woodland Hills, Los Angeles County, California, USA
    Posts
    9,643
    Rep Power
    4248
    Actually it is not an OS specific issue. It can happen on just about any OS :).
    Up the Irons
    What Would Jimi Do? Smash amps. Burn guitar. Take the groupies home.
    "Death Before Dishonour, my Friends!!" - Bruce D ickinson, Iron Maiden Aug 20, 2005 @ OzzFest
    Down with Sharon Osbourne

    "I wouldn't hire a butcher to fix my car. I also wouldn't hire a marketing firm to build my website." - Nilpo
  28. #15
  29. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2006
    Posts
    43
    Rep Power
    9
    Code:
    ptr = malloc(some_size * sizeof(MyStruct));
    malloc returns void* - on my c++ compiler it wouldnt compile this part of the code because it is dangerous

    causes error: cannot convert from 'void *' to 'MyStruct *'

    so this code won't compile in c++ but in c it will compile and that is bad because you can overflow the array

    example...
    Code:
    int main()
    {
    	typedef struct
    	{
    		char x;
    	} MyStruct;
    	MyStruct *ptr;
    
    	ptr =  (MyStruct*) malloc(2*sizeof(MyStruct)); // allocates array of 2 structs
    
    	ptr[0].x = 'a';
    	ptr[1].x = 'b';
    
    
    	/* there is no structure at this adress - but your compiler cannot warn you
    	which means you will overwrite memory that was not allocated to this array */
    	ptr[2].x = 'c';
    
    
    	free(ptr);
    }
    if this is the problem fix it by not using dynamic memory
Page 1 of 2 12 Last
  • Jump to page:

IMN logo majestic logo threadwatch logo seochat tools logo