When it will be freed?

T

tfelb

You know that goto's required by doped silicon are pretty good smokes

Hmm I did what someone said..I coded a strpad function with the
parameters (oldstring, newLen, given character) (make the orginal
string bigger (the newLen is now the len of the new string), copy the
old string into /new/ string and fill the rest with '.' chars.
But a heap corruption takes place?

1. In the function strpad, I terminate the new string with '\0'. Maybe
thats the cause?

One solution for the problem I guess is if i set p after the printf
call to NULL ( p = NULL )

int main(int argc, char **argv)
{

   char text[] = "This is a test sentence);
   char *p = strpad(text,30,'.');

   if(p)
   {
   printf("%s",p);
   free(p); / cause a memory heap corruption ? */
   }



}- Zitierten Text ausblenden -

- Zitierten Text anzeigen -

Oh I see a typo. The corrected version is

int main(int argc, char **argv)
{

char text[] = "This is a test sentence";
char *p = strpad(text,30,'.');


if(p)
{
printf("%s",p);
free(p); / cause a memory heap corruption ? */
}



}
 
J

James Kuyper

tfelb wrote:
....
Hmm I did what someone said..I coded a strpad function with the
parameters (oldstring, newLen, given character) (make the orginal
string bigger (the newLen is now the len of the new string), copy the
old string into /new/ string and fill the rest with '.' chars.
But a heap corruption takes place?

1. In the function strpad, I terminate the new string with '\0'. Maybe
thats the cause?

How should we know? Without the source code for strpad(), it's pretty
difficult to figure out why it's malfunctioning.
 
T

tfelb

tfelb wrote:

...



How should we know? Without the source code for strpad(), it's pretty
difficult to figure out why it's malfunctioning.

Again I know casting malloc is crazy but VS 2005 TS won't compile
without the cast.

char *strpad(char *s1, int len, const char *s2)
{
int i = 0;
char *p, *start;
size_t rest;

if((p = (char *)malloc(len+1)) == NULL)
return NULL;
memmove(p,s1,strlen(s1));
start = p;
p += strlen(s1)-1;
rest = len - strlen(s1);
while(i++ < rest)
memmove(p+i,s2,strlen(s2));

*(start+strlen(s1)+i) = '\0';




return start;



}
 
B

Ben Bacarisse

tfelb said:
Again I know casting malloc is crazy but VS 2005 TS won't compile
without the cast.

Maybe it thinks you are writing C++ rather than C?
char *strpad(char *s1, int len, const char *s2)
{
int i = 0;
char *p, *start;
size_t rest;

if((p = (char *)malloc(len+1)) == NULL)
return NULL;
memmove(p,s1,strlen(s1));

Both strcat and memcpt would be better. memmove is for regions that
might overlap.
start = p;
p += strlen(s1)-1;
rest = len - strlen(s1);

rest is unsigned. Do you know what happens if strlen(s1) > len?
while(i++ < rest)
memmove(p+i,s2,strlen(s2));

Odd indentation. This is a very odd thing to do. Did you intend s2
be a single-character string? If so, it is much safer to pass a char
and pad using memset. This can write beyond the end of your malloced
space the very first time round the loop.
*(start+strlen(s1)+i) = '\0';

I find start[strlen(s1) + i] = '\0'; clearer notation, but since the
purpose is to ensure that the string is terminated, you should write

start[len] = '\0';

anyway. If all the loops and calculations work out this should be the
same, but why take the risk and why make everyone who reads the code
have to check?

Lets check, though, anyway. The while loop ends when i++ == rest.
I.e. with i == rest + 1. If we plug that in here you get:

start[strlen(s1) + rest + 1] = '\0';
or
start[strlen(s1) + len - strlen(s1) + 1] = '\0';
or
start[len + 1] = '\0';

Ah. Not correct.
 
B

Ben Bacarisse

In another post, strpad (not an ideal name, BTW, since strxxx names
are reserved for the C library) takes a const char * in the third
position. Passing a char will go horribly wrong. You compiler should
be complaining about it.

Passing a char is a good idea as I said in my other reply, but the
function must know to expect one!
 
T

tfelb

In another post, strpad (not an ideal name, BTW, since strxxx names
are reserved for the C library) takes a const char * in the third
position.  Passing a char will go horribly wrong.  You compiler should
be complaining about it.

Passing a char is a good idea as I said in my other reply, but the
function must know to expect one!

Ah I see, i think it takes a very long time to be a good c
programmer..I will do my best but if i look at your answer i think my /
best/ is at a very low level. (it sounds like complaining) I love C
and my will is strong to be a better programmer!

the call of the function is char *p = strpad(text,30,"."); i made a
typo.

Anyway i can't figure out why i got a heap corruption?

Thank you!

Tom
 
K

Keith Thompson

tfelb said:
Again I know casting malloc is crazy but VS 2005 TS won't compile
without the cast.
[...]

Then you're probably invoking it as a C++ compiler. I'm sure there's
a way to invoke it as a C compiler, but I don't know what it is.
 
T

tfelb

Keith Thompson said:
tfelb said:
Again I know casting malloc is crazy but VS 2005 TS won't compile
without the cast. [...]

Then you're probably invoking it as a C++ compiler.  I'm sure
there's a way to invoke it as a C compiler, but I don't know what
it is.

.c file suffix, /Za /W4

--
Richard Heathfield <http://www.cpax.org.uk>
Email: -http://www. +rjh@
Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
"Usenet is a strange place" - dmr 29 July 1999

Thank you for your answers! But again I want to figure out why I had a
heap corruption? Any help? Or is that a fool question?

Thanks,
 
J

James Kuyper

tfelb wrote:
....
Thank you for your answers! But again I want to figure out why I had a
heap corruption? Any help? Or is that a fool question?

Well, as Ben Bacarisse has already pointed out, your function writes one
position past the end of the array. Whenever a program does that, it's
behavior is undefined, because some other part of the program might be
using that memory for some other purpose, and changing the value stored
there will cause that other part of the program to malfunction

In this particular case, that memory was probably being used by the
malloc() family of functions to store information relevant to the
management of the heap, so making arbitrary changes to that memory
causes heap corruption. This is a very common symptom when writing just
before or just after the end of a dynamically allocated piece of memory.
 
T

tfelb

tfelb wrote:

...


Well, as Ben Bacarisse has already pointed out, your function writes one
position past the end of the array. Whenever a program does that, it's
behavior is undefined, because some other part of the program might be
using that memory for some other purpose, and changing the value stored
there will cause that other part of the program to malfunction

In this particular case, that memory was probably being used by the
malloc() family of functions to store information relevant to the
management of the heap, so making arbitrary changes to that memory
causes heap corruption. This is a very  common symptom when writing just
before or just after the end of a dynamically allocated piece of memory.

Ok, thanks I will fix it!
 
M

Mark Wooding

Ian Collins said:
Keep your functions short and this won't be an issue.

.... but you end up with lots of state to cart about between cooperating
functions.

I'm a proud advocate of goto. I tend to write functions in one of these
ways:

* Simple functions which can't fail: just return the right thing when
I find it.

* Relatively simple functions which might fail, and which allocate at
most one resource as the last possible failure: early-return on
errors.

* Hairy functions which allocate several resources: a toss-up between
a few techniques.

-- A sequence of fail_... labels at the end of the function:
failures goto the appropriate label, which disposes of the
relevant resources in reverse order, and finally return a
failure code. Successful exits don't use the failure codepath:
this is useful for constructor-type functions.

-- A sequence of done_... labels at the end of the function:
failures goto the appropriate label, as above. Successful
exits use the same codepath; the exit code is stashed in a
variable, which is finally returned. This is useful for
functions which aren't meant to construct things but rather
just do a job.

-- A single done or fail label (corresponding to one of the above
cases) which deallocates resources conditionally according to
whether they were actually allocated. This gets used when the
pattern of resource allocations is too hairy to deal with
otherwise.

I'm also not afraid to use goto in other circumstances, e.g.,

for (i = 0; i < N(footab); i++) {
if (strcmp(name, footab.name) == 0)
goto found;
}
moan("unknown foo name `%s'", name);
return (-1);
found:;

and I don't mind backwards-goto for loops which I strongly expect to
only happen once in usual cases -- the idea being to present the code to
the reader as being essentially linear but with a special case, rather
than as being primarily iterative. The presence of a label called
`again' should be enough to let the reader know that something hinky
might happen.

-- [mdw]
 
I

Ian Collins

Mark said:
.... but you end up with lots of state to cart about between cooperating
functions.

There's nothing wrong with that. If anything it's an advantage.
 
M

Mark Wooding

Ian Collins said:
There's nothing wrong with that. If anything it's an advantage.

Actually, I'm not sure I understand how this works after all. I've
just found a function of mine: it's 58 lines long, allocates five
things, and needs to clear them all away if anything goes wrong.

To save the usual problems with abstractness, you can see the code at
http://git.distorted.org.uk/gitweb/~mdw/tripe/blob/HEAD:/server/peer.c#l714
if you like.

If you know of a better way to structure a function like this, I'd be
honestly interested to know it. I can't think of anything that wouldn't
make it worse.

-- [mdw]
 
I

Ian Collins

Mark said:
Actually, I'm not sure I understand how this works after all. I've
just found a function of mine: it's 58 lines long, allocates five
things, and needs to clear them all away if anything goes wrong.

If you know of a better way to structure a function like this, I'd be
honestly interested to know it. I can't think of anything that wouldn't
make it worse.
For starters, each goto can be replaced with a nested if/else by
inverting the logic of the if. Once the function has some structure
(which makes it easier on the eye, one can see each layer of the onion),
it can be further refactored. Assuming is has decent unit tests!

For example,

if (f) goto tidy 1

// do stuff
return (p);

tidy 1:

DESTROY(p);
return (0);

can be changed to

if (f != 0)
{
//do stuff
return p;
}

DESTROY(p);
return NULL;
 
N

Nate Eldredge

Ian Collins said:
For starters, each goto can be replaced with a nested if/else by
inverting the logic of the if. Once the function has some structure
(which makes it easier on the eye, one can see each layer of the onion),
it can be further refactored. Assuming is has decent unit tests!

For example,

if (f) goto tidy 1

// do stuff
return (p);

tidy 1:

DESTROY(p);
return (0);

can be changed to

if (f != 0)
{
//do stuff
return p;
}

DESTROY(p);
return NULL;

If there's only one thing to do, of course this is fine. But I don't
see how it extends. Can you show how you would use this technique when
there are five things to be done, and if any of them fails then all the
preceding ones must be cleaned up?

I also don't follow your remark about refactoring and unit tests.
Again, a more extended example would be helpful.
 
M

Mark Wooding

Ian Collins said:
For starters, each goto can be replaced with a nested if/else by
inverting the logic of the if. Once the function has some structure
(which makes it easier on the eye, one can see each layer of the onion),
it can be further refactored.

Ugh! The cure is worse than the disease! That lets the error handling
take over the logic of the function, which is an essentially linear
sequence of steps.

-- [mdw]
 
I

Ian Collins

Nate said:
If there's only one thing to do, of course this is fine. But I don't
see how it extends. Can you show how you would use this technique when
there are five things to be done, and if any of them fails then all the
preceding ones must be cleaned up?
Sure:

if (f) goto tidy 1

allocate( something );
// do stuff

if (f) goto tidy 2

// do stuff

return (p);

tidy 2:

free( something );

tidy 1:

DESTROY(p);
return (0);

can be changed to

if (f != 0)
{
allocate( something );
// do stuff

if (f != 0)
{
//do stuff

return p;
}
free( something );
}

DESTROY(p);
return NULL;
I also don't follow your remark about refactoring and unit tests.
Again, a more extended example would be helpful.

In order to confidently refactor a function, one has to be able to
verify it's behaviour remains unchanged, preferably after each
modification. Without tests, that's not feasible.
 
I

Ian Collins

Mark said:
Ugh! The cure is worse than the disease! That lets the error handling
take over the logic of the function, which is an essentially linear
sequence of steps.
Does it? In my opinion, it makes it clear which operation depends on
what. Scanning indented code is easier and looking for gotos and their
matching labels.

I also think it helps in extracting layers of the onion into functions
if the nesting is too deep, or the logic unclear.

I've always found structured code easier to read, maintain and refactor.
 
N

Nate Eldredge

Ian Collins said:
Sure:

if (f) goto tidy 1

allocate( something );
// do stuff

if (f) goto tidy 2

// do stuff

return (p);

tidy 2:

free( something );

tidy 1:

DESTROY(p);
return (0);

can be changed to

if (f != 0)
{
allocate( something );
// do stuff

if (f != 0)
{
//do stuff

return p;
}
free( something );
}

DESTROY(p);
return NULL;

I see.

I find it confusing that the "normal" return from the function is buried
within many layers of `if', and if there are several tests to be done,
horizontal space becomes an issue.

Your approach has the property that allocations and deallocations appear
nicely paired, but actually I think that's a disadvantage here. At
first glance it looks as if the function is intended to allocate
something, use it, and then free it. In fact, that happens only in case
of an error; normally it allocates something and does not free it.

Of the two, I still prefer the one with the gotos. For error handling,
the fact that the gotos are always forward, that they are taken only in
the "exceptional" case, and that at most one is ever taken per call to
the function takes away the "spaghetti" aspect for me. I find the use
of gotos in this way to be pretty idiomatic, and I still haven't seen an
approach in C that I prefer. (C++, of course, offers destructors, which
is really the ideal way to do this.)
In order to confidently refactor a function, one has to be able to
verify it's behaviour remains unchanged, preferably after each
modification. Without tests, that's not feasible.

Ah, and by "refactor" you mean "change the code in some way without
changing the effect." I tend to think of the Forth meaning of "break
apart into smaller functions", and it wasn't clear to me how the latter
was feasible here.
 
I

Ian Collins

Nate said:
I find it confusing that the "normal" return from the function is buried
within many layers of `if', and if there are several tests to be done,
horizontal space becomes an issue.

But you only reach the treasure after passing all the challenges!
Your approach has the property that allocations and deallocations appear
nicely paired, but actually I think that's a disadvantage here. At
first glance it looks as if the function is intended to allocate
something, use it, and then free it. In fact, that happens only in case
of an error; normally it allocates something and does not free it.

That's a perfectly valid assumption. I was fiddling with what was
there. If it were may code, I would consider building up the final
object from local variables with a return NULL if building any one failed.
Of the two, I still prefer the one with the gotos. For error handling,
the fact that the gotos are always forward, that they are taken only in
the "exceptional" case, and that at most one is ever taken per call to
the function takes away the "spaghetti" aspect for me. I find the use
of gotos in this way to be pretty idiomatic, and I still haven't seen an
approach in C that I prefer. (C++, of course, offers destructors, which
is really the ideal way to do this.)

I'm glad someone else said that, so I don't get accused of a C++ bias!
Ah, and by "refactor" you mean "change the code in some way without
changing the effect." I tend to think of the Forth meaning of "break
apart into smaller functions", and it wasn't clear to me how the latter
was feasible here.

"break apart into smaller functions" is a common refacroring (Extract
Method). Just about anywhere there is a new scope, the code in that
scope could be extracted if it has a distinct purpose. I tend to favour
the rule "if a block of code needs a comment, extract it.
 

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,483
Members
44,901
Latest member
Noble71S45

Latest Threads

Top