Discuss Scorpy's Puzzle of the Week 2007-07-18 in the C Programming forum on Dev Shed. Scorpy's Puzzle of the Week 2007-07-18 C programming forum discussing all C derivatives, including C#, C++, Object-C, and even plain old vanilla C. These languages are low level languages, and used on projects such as device drivers, compilers, and even whole computer operating systems.
Location: Woodland Hills, Los Angeles County, California, USA
Posts: 9,406
Time spent in forums: 2 Months 10 h 17 m 19 sec
Reputation Power: 4080
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.
__________________ 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
Last edited by Scorpions4ever : July 19th, 2007 at 12:08 PM.
Posts: 5,964
Time spent in forums: 2 Months 3 Weeks 2 Days 12 h 47 m 19 sec
Warnings Level: 10
Number of bans: 1
Reputation Power: 4851
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 ***.
Posts: 3,907
Time spent in forums: 2 Months 3 Weeks 4 Days 1 h 39 m 56 sec
Reputation Power: 1774
> 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.
Posts: 648
Time spent in forums: 6 Days 21 h 32 m 25 sec
Reputation Power: 74
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.
Location: Woodland Hills, Los Angeles County, California, USA
Posts: 9,406
Time spent in forums: 2 Months 10 h 17 m 19 sec
Reputation Power: 4080
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 12:10 PM.
Posts: 2,270
Time spent in forums: 1 Month 2 Weeks 4 Days 15 h 34 m 57 sec
Reputation Power: 1735
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.
Posts: 989
Time spent in forums: 2 Weeks 2 Days 22 h 45 m 6 sec
Reputation Power: 362
if for some reason you end up with some_size == 0 you cant be sure the return is a valid pointer.
Quote:
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.
Posts: 4,378
Time spent in forums: 1 Month 2 Weeks 2 Days 11 h 5 m 4 sec
Reputation Power: 1509
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.
Posts: 3,907
Time spent in forums: 2 Months 3 Weeks 4 Days 1 h 39 m 56 sec
Reputation Power: 1774
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.
Posts: 43
Time spent in forums: 22 h 38 m 31 sec
Reputation Power: 7
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
Del.icio.us
Digg
Google
Spurl
Blink
Furl
Simpy
Y! MyWeb
. Assume that I'll free() it later down the line.
Linear Mode
