18 Mar, 2013, arholly wrote in the 1st comment:
Votes: 0
Hello:
I know there is a memory leak in this code (str) and I thought adding a free to it would take care of it, but then it crashes when I go to startup saying there is an invalid pointer. What am I doing wrong?

void name_replacer(char *format, char *first, char *last, char *out)
{
char buf[MSL]={'\0'};
char *str;
char *i;
char *point;

if(format == NULL)
return;

point = buf;
str = str_dup(format);

// we should never reach this if format is null.
if(str == NULL)
return;

/* replace $f with first and $l with last. */
while ( *str != '\0' )
{
if ( *str != '$' )
{
*point++ = *str++;
continue;
}
++str;

switch ( *str )
{
default: bug( "Rand_name: bad code %d.", *str );
i = " Dave "; break;
case 'f': i = first;
break;
case 'l': i = last;
break;
case ' ': i = "$";
break;
case '$': i = "$";
break;
}

++str;
while ( ( *point = *i ) != '\0' )
++point, ++i;
}
*point++ = '\0';

/* Removed to try to fix name changing bug.
return str_dup(buf);
*/
strcpy(out, buf);

free((void *)str);

return;
}
18 Mar, 2013, Davenge wrote in the 2nd comment:
Votes: 0
What does GDB/valgrind say? That's always a good place to start.

edit: I only ask because I don't know how your codebase deals with things, but what I work in, a lot of the memory allocation/delocation stuff is handled by MACRO functions so I don't really need to think about it too much as long as I'm mindful of my pointers and the data they point to doesn't get "not disposed of" when done. Perhaps we can both learn something here.

Double edit: and I suppose I should have paid attention to the threat topic tree a bit better… still
18 Mar, 2013, arholly wrote in the 3rd comment:
Votes: 0
Sure does.
Quote
*** glibc detected *** /home/m243bra/ptdev/src/project: munmap_chunk(): invalid pointer: 0x0838528e ***
======= Backtrace: =========
/lib/libc.so.6(cfree+0x188)[0x291d88]
/home/m243bra/ptdev/src/project[0x80b51a3]
/home/m243bra/ptdev/src/project[0x80b52df]
/home/m243bra/ptdev/src/project[0x80b2227]
/home/m243bra/ptdev/src/project[0x80adf77]
/home/m243bra/ptdev/src/project[0x80ae4b5]
/home/m243bra/ptdev/src/project[0x80ad19b]
/home/m243bra/ptdev/src/project[0x80aa97d]
/home/m243bra/ptdev/src/project[0x809d579]
/lib/libc.so.6(__libc_start_main+0xdc)[0x239ebc]

Program received signal SIGABRT, Aborted.
0x002057f2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
(gdb) bt
#0 0x002057f2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1 0x0024ce30 in raise () from /lib/libc.so.6
#2 0x0024e741 in abort () from /lib/libc.so.6
#3 0x002858cb in __libc_message () from /lib/libc.so.6
#4 0x00291d88 in free () from /lib/libc.so.6
#5 0x080b51a3 in name_replacer (format=0x8218b90 "policewoman officer $f", first=0x8162ebc "Rachel", last=0x8163182 "Bevill", out=0xbfff9610 "policewoman officer Rachel")
at db.c:4572
#6 0x080b52df in rand_name (ch=0x8384b20) at db.c:4658
#7 0x080b2227 in create_mobile (pMobIndex=0x8218980) at db.c:3380
#8 0x080adf77 in reset_room (pRoom=0x821ae48) at db.c:2024
#9 0x080ae4b5 in reset_area (pArea=0x8218320) at db.c:2192
#10 0x080ad19b in area_update () at db.c:1563
#11 0x080aa97d in boot_db () at db.c:461
#12 0x0809d579 in main (argc=2, argv=0xbfffd854) at comm.c:647
(gdb) frame 0
#0 0x002057f2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
(gdb) list
582
583 purgeExtractedWorldData();
584 log_string("Project Twilight has completed its cleanup procedure and may now shutdown.");
585 return;
586 }
587
588 int main( int argc, char **argv )
589 {
590 struct timeval now_time;
591 bool fCopyOver = FALSE;
(gdb) info loca
No symbol table info available.
(gdb) frame 1
#1 0x0024ce30 in raise () from /lib/libc.so.6
(gdb) list
592
593 /*
594 * Init time.
595 */
596 gettimeofday( &now_time, NULL );
597 current_time = (time_t) now_time.tv_sec;
598 strncpy( str_boot_time, ctime( &current_time ), sizeof(str_boot_time));
599
600 /*
601 * Reserve one channel for our use.
(gdb) info local
No symbol table info available.
(gdb) frame 2
#2 0x0024e741 in abort () from /lib/libc.so.6
(gdb) list
602 */
603 if ( ( fpReserve = fopen( NULL_FILE, "r" ) ) == NULL )
604 {
605 perror( NULL_FILE );
606 exit( 1 );
607 }
608
609 /*
610 * Get the port number and initial log file number.
611 */
(gdb) info local
No symbol table info available.
(gdb) frame 3
#3 0x002858cb in __libc_message () from /lib/libc.so.6
(gdb) list
612 port = MUD_PORT;
613 if ( argc > 1 )
614 {
615 if ( !is_number( argv[1] ) )
616 {
617 fprintf( stderr, "Usage: %s [port #] [first log #]\n", argv[0] );
618 exit( 1 );
619 }
620 else if ( ( port = atoi( argv[1] ) ) <= 1024 )
621 {
(gdb) info local
No symbol table info available.
(gdb) frame 4
#4 0x00291d88 in free () from /lib/libc.so.6
(gdb) list
622 fprintf( stderr, "Port number must be above 1024.\n" );
623 exit( 1 );
624 }
625
626 /* Are we recovering from a copyover? */
627 if (argv[3] && argv[3][0])
628 {
629 fCopyOver = TRUE;
630 control = atoi(argv[3]);
631 }
(gdb) info local
No symbol table info available.
(gdb) frame 5
#5 0x080b51a3 in name_replacer (format=0x8218b90 "policewoman officer $f", first=0x8162ebc "Rachel", last=0x8163182 "Bevill", out=0xbfff9610 "policewoman officer Rachel")
at db.c:4572
4572 free((void *)str);
(gdb) list
4567 /* Removed to try to fix name changing bug.
4568 return str_dup(buf);
4569 */
4570 strcpy(out, buf);
4571
4572 free((void *)str);
4573
4574 return;
4575 }
4576
(gdb) info local
buf = "policewoman officer Rachel", '\000' <repeats 4069 times>
str = 0x838528e ""
i = 0x8162ec2 ""
point = 0xbfff85f7 ""
__FUNCTION__ = "name_replacer"
(gdb) frame 6
#6 0x080b52df in rand_name (ch=0x8384b20) at db.c:4658
4658 name_replacer(ch->pIndexData->player_name, first, last, out);
(gdb) list
4653 first = name_select(male_names);
4654 last = name_select(last_names);
4655 }
4656
4657 PURGE_DATA(ch->name);
4658 name_replacer(ch->pIndexData->player_name, first, last, out);
4659 ch->name = str_dup(out);
4660 PURGE_DATA(ch->short_descr);
4661 name_replacer(ch->pIndexData->short_descr, first, last, out);
4662 ch->short_descr = str_dup(out);
(gdb) info local
first = 0x8162ebc "Rachel"
last = 0x8163182 "Bevill"
out = "policewoman officer Rachel", '\000' <repeats 16357 times>
__FUNCTION__ = "rand_name"
18 Mar, 2013, Davenge wrote in the 4th comment:
Votes: 0
Not gonna lie, I don't see why a pointer duplicated in that same method would cause a crash when free'd.
18 Mar, 2013, Vigud wrote in the 5th comment:
Votes: 0
Allocate memory and free it "in pairs": one malloc() – one free(). So with that in mind - did you malloc() anything that you're trying to free? What does str_dup() really do?
18 Mar, 2013, arholly wrote in the 6th comment:
Votes: 0
char *_str_dup(const char *str, const char *file, const char *function, int line)
{
if(IS_NULLSTR(str))
{
log_string(Format("_str_dup: %s:%s:%d called with NULL values!", file, function, line ) );
return NULL;
}
return strdup(str); // relay to our actual strdup!
}
18 Mar, 2013, Vigud wrote in the 7th comment:
Votes: 0
That's _str_dup(). Also, did you try to free(str) instead of free((void *)str)?
18 Mar, 2013, arholly wrote in the 8th comment:
Votes: 0
#define str_dup(str) _str_dup(str, __FILE__, __FUNCTION__, __LINE__)

Yes, and got pretty much the same thing:
Quote
*** glibc detected *** /home/m243bra/ptdev/src/project: munmap_chunk(): invalid pointer: 0x0838528e ***
warning: .dynamic section for "/lib/libgcc_s.so.1" is not at the expected address
warning: difference appears to be caused by prelink, adjusting expectations
======= Backtrace: =========
/lib/libc.so.6(cfree+0x188)[0x419d88]
/home/m243bra/ptdev/src/project[0x80b51a3]
/home/m243bra/ptdev/src/project[0x80b52df]
/home/m243bra/ptdev/src/project[0x80b2227]
/home/m243bra/ptdev/src/project[0x80adf77]
/home/m243bra/ptdev/src/project[0x80ae4b5]
/home/m243bra/ptdev/src/project[0x80ad19b]
/home/m243bra/ptdev/src/project[0x80aa97d]
/home/m243bra/ptdev/src/project[0x809d579]
/lib/libc.so.6(__libc_start_main+0xdc)[0x3c1ebc]
/home/m243bra/ptdev/src/project[0x80491a1]
======= Memory map: ========
00110000-0011b000 r-xp 00000000 fd:01 56263827 /lib/libgcc_s-4.1.2-20080825.so.1
0011b000-0011c000 rw-p 0000a000 fd:01 56263827 /lib/libgcc_s-4.1.2-20080825.so.1
00205000-00220000 r-xp 00000000 fd:01 56263146 /lib/ld-2.5.so
00220000-00221000 r–p 0001a000 fd:01 56263146 /lib/ld-2.5.so
00221000-00222000 rw-p 0001b000 fd:01 56263146 /lib/ld-2.5.so
002ed000-002ee000 r-xp 002ed000 00:00 0 [vdso]
00383000-003aa000 r-xp 00000000 fd:01 56263779 /lib/libm-2.5.so
003aa000-003ab000 r–p 00026000 fd:01 56263779 /lib/libm-2.5.so
003ab000-003ac000 rw-p 00027000 fd:01 56263779 /lib/libm-2.5.so
003ac000-00503000 r-xp 00000000 fd:01 56263772 /lib/libc-2.5.so
00503000-00505000 r–p 00156000 fd:01 56263772 /lib/libc-2.5.so
00505000-00506000 rw-p 00158000 fd:01 56263772 /lib/libc-2.5.so
00506000-00509000 rw-p 00506000 00:00 0
08048000-08196000 r-xp 00000000 fd:01 69140583 /home/m243bra/ptdev/src/project
08196000-08198000 rw-p 0014e000 fd:01 69140583 /home/m243bra/ptdev/src/project
08198000-08396000 rw-p 08198000 00:00 0 [heap]
b7ff5000-b7ff7000 rw-p b7ff5000 00:00 0
b7ffc000-b7ffd000 rw-p b7ffc000 00:00 0
bffe9000-bfffe000 rw-p bffe8000 00:00 0
Program received signal SIGABRT, Aborted.
0x002057f2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
(gdb) bt
#0 0x002057f2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1 0x003d4e30 in raise () from /lib/libc.so.6
#2 0x003d6741 in abort () from /lib/libc.so.6
#3 0x0040d8cb in __libc_message () from /lib/libc.so.6
#4 0x00419d88 in free () from /lib/libc.so.6
#5 0x080b51a3 in name_replacer (format=0x8218b90 "policewoman officer $f", first=0x81630aa "Lily", last=0x8163392 "Bruns", out=0xbfff9610 "policewoman officer Lily")
at db.c:4572
#6 0x080b52df in rand_name (ch=0x8384b20) at db.c:4658
#7 0x080b2227 in create_mobile (pMobIndex=0x8218980) at db.c:3380
#8 0x080adf77 in reset_room (pRoom=0x821ae48) at db.c:2024
#9 0x080ae4b5 in reset_area (pArea=0x8218320) at db.c:2192
#10 0x080ad19b in area_update () at db.c:1563
#11 0x080aa97d in boot_db () at db.c:461
#12 0x0809d579 in main (argc=2, argv=0xbfffd854) at comm.c:647
(gdb) list
582
583 purgeExtractedWorldData();
584 log_string("Project Twilight has completed its cleanup procedure and may now shutdown.");
585 return;
586 }
587
588 int main( int argc, char **argv )
589 {
590 struct timeval now_time;
591 bool fCopyOver = FALSE;
(gdb) info locals
No symbol table info available.
(gdb) frame 1
#1 0x003d4e30 in raise () from /lib/libc.so.6
(gdb) list
592
593 /*
594 * Init time.
595 */
596 gettimeofday( &now_time, NULL );
597 current_time = (time_t) now_time.tv_sec;
598 strncpy( str_boot_time, ctime( &current_time ), sizeof(str_boot_time));
599
600 /*
601 * Reserve one channel for our use.
(gdb) info local
No symbol table info available.
(gdb) frame 2
#2 0x003d6741 in abort () from /lib/libc.so.6
(gdb) list
602 */
603 if ( ( fpReserve = fopen( NULL_FILE, "r" ) ) == NULL )
604 {
605 perror( NULL_FILE );
606 exit( 1 );
607 }
608
609 /*
610 * Get the port number and initial log file number.
611 */
(gdb) info local
No symbol table info available.
(gdb) frame 3
#3 0x0040d8cb in __libc_message () from /lib/libc.so.6
(gdb) list
612 port = MUD_PORT;
613 if ( argc > 1 )
614 {
615 if ( !is_number( argv[1] ) )
616 {
617 fprintf( stderr, "Usage: %s [port #] [first log #]\n", argv[0] );
618 exit( 1 );
619 }
620 else if ( ( port = atoi( argv[1] ) ) <= 1024 )
621 {
(gdb) info local
No symbol table info available.
(gdb) frame 4
#4 0x00419d88 in free () from /lib/libc.so.6
(gdb) list
622 fprintf( stderr, "Port number must be above 1024.\n" );
623 exit( 1 );
624 }
625
626 /* Are we recovering from a copyover? */
627 if (argv[3] && argv[3][0])
628 {
629 fCopyOver = TRUE;
630 control = atoi(argv[3]);
631 }
(gdb) info local
No symbol table info available.
(gdb) frame 5
#5 0x080b51a3 in name_replacer (format=0x8218b90 "policewoman officer $f", first=0x81630aa "Lily", last=0x8163392 "Bruns", out=0xbfff9610 "policewoman officer Lily")
at db.c:4572
4572 free(str);
(gdb) list
4567 /* Removed to try to fix name changing bug.
4568 return str_dup(buf);
4569 */
4570 strcpy(out, buf);
4571
4572 free(str);
4573
4574 return;
4575 }
4576
(gdb) info local
buf = "policewoman officer Lily", '\000' <repeats 4071 times>
str = 0x838528e ""
i = 0x81630ae ""
point = 0xbfff85f5 ""
__FUNCTION__ = "name_replacer"
18 Mar, 2013, Vigud wrote in the 9th comment:
Votes: 0
What does ++str do?
18 Mar, 2013, quixadhal wrote in the 10th comment:
Votes: 0
Very hard to follow without proper indentation. :(

++str increments the pointer. Since you're using it several times inside the while(*str++ != '\0') loop, I would guess at one point it's incrementing off the end of the string.

But, however you're doing things… you're changing the value of str and then at the bottom, you're trying to free the NEW value, which is not the original address of the malloc().

Essentially, you're doing "thing = malloc(); thing = 748392; free(thing);"
18 Mar, 2013, Davenge wrote in the 11th comment:
Votes: 0
quixadhal said:
Very hard to follow without proper indentation. :(

++str increments the pointer. Since you're using it several times inside the while(*str++ != '\0') loop, I would guess at one point it's incrementing off the end of the string.

But, however you're doing things… you're changing the value of str and then at the bottom, you're trying to free the NEW value, which is not the original address of the malloc().

Essentially, you're doing "thing = malloc(); thing = 748392; free(thing);"


I don't see where he redefines *str. Even so, that would cause a memory leak, but certainly not a crash.

Edit:
Hah… nevermind.
18 Mar, 2013, Davenge wrote in the 12th comment:
Votes: 0
In that case, why not then just copy format into a new character array and not worrying about dealing with pointers and memory leaks. You can do the same thing.

Edit: Or just do a straight *str = *format. Don't duplicate it. There's really no need.
18 Mar, 2013, roh-bane wrote in the 13th comment:
Votes: 0
You're getting a pointer to new memory and storing it in str(0x00001000) for example), you're then incrementing str so it ends up pointing at the end of the new memory, and then trying to free str, which now points to whatever memory str started out with + the length of str(let's say 10) so you're trying to free (0x1010 instead of 0x00001000). You need to track the original start of the memory provided by str_dup, try something like:

At line 7 add:
char *toFree;

At line 13 add:
toFree = str;

change the free(str) to free(toFree);
19 Mar, 2013, Davenge wrote in the 14th comment:
Votes: 0
He can do that, or he can just set *str = to *format. No str_dup. He never uses *str to change data it points to, unless I missed something. Then he doesn't need to do anything.
19 Mar, 2013, Rarva.Riendf wrote in the 15th comment:
Votes: 0
Davenge said:
He can do that, or he can just set *str = to *format. No str_dup. He never uses *str to change data it points to, unless I missed something. Then he doesn't need to do anything.


Depends how was allocated format in the first place as well.
There are many solutions, just be sure to put comments on your rmethod to know what you should expect from it :) (modification of pointer, having to free the result etc )
When returning a "string" there is also the possibility to use a static one so you dont have to think about freeing memory (but then have to think of not calling it twice in a row before storing the first result).
Trade off.
19 Mar, 2013, quixadhal wrote in the 16th comment:
Votes: 0
So, because I was bored… here's a rewrite of the original routine that allocates a new string buffer and returns it, unless the string doesn't contain any tokens to be replaced… in which case it returns the original.

It may have bugs. I haven't tested it… but it shows a different way of doing this which might be more efficient, and at least should be educational. :)

char *replace_name_tokens(const char *str, const char *first_name, const char *last_name)
{
char *result = NULL;
char *match = NULL;
char *current = NULL;

if(!str || !*str)
return str;

if(!(match = strchr(str, (int)'$')))
return str;

// The tokens recognized are $f for first name, and $l for last name.
// The original function also replace $$ with $, and "$ " with $.
current = str;
while(match = strchr(current, (int)'$')) {
if(!result) {
result = strndup(current, (match - current));
} else {
result = realloc(result, strlen(result) + (match - current + 1) * sizeof(char));
strncat(result, current, match - current);
}

current = match + 1;
switch(*current) {
case '\0':
// A $ at the end of the string is a false match.
break;
case 'f':
result = realloc(result, (strlen(result) + strlen(first) + 1) * sizeof(char));
strcat(result, first);
break;
case 'l':
result = realloc(result, (strlen(result) + strlen(last) + 1) * sizeof(char));
strcat(result, last);
break;
case '$':
result = realloc(result, (strlen(result) + 2) * sizeof(char));
strcat(result, "$");
break;
case ' ':
// Should this really be a $ ???
result = realloc(result, (strlen(result) + 2) * sizeof(char));
strcat(result, "$");
break;
default:
// Copy it verbatum.
result = realloc(result, (strlen(result) + 3) * sizeof(char));
strncat(result, match, 2);
break;
}
current++;
}
leftover = strlen(current);
if(leftover > 0) {
result = realloc(result, (strlen(result) + leftover + 1) * sizeof(char));
strcat(result, current);
}
return result;
}
19 Mar, 2013, Vigud wrote in the 17th comment:
Votes: 0
Quote
(int)'$'
Type of '$' is already int.
19 Mar, 2013, Davenge wrote in the 18th comment:
Votes: 0
Rarva.Riendf said:
Davenge said:
He can do that, or he can just set *str = to *format. No str_dup. He never uses *str to change data it points to, unless I missed something. Then he doesn't need to do anything.


Depends how was allocated format in the first place as well.
There are many solutions, just be sure to put comments on your rmethod to know what you should expect from it :) (modification of pointer, having to free the result etc )
When returning a "string" there is also the possibility to use a static one so you dont have to think about freeing memory (but then have to think of not calling it twice in a row before storing the first result).
Trade off.


In some situation in C, and I'm not 100% sure which, the compiler won't let you return a non-static string.
19 Mar, 2013, Kaz wrote in the 19th comment:
Votes: 0
Vigud said:
What does ++str do?


Is the correct answer.

. +-+-+-+-+-+-+-+-+-+
. | | | | | | | | | |
. +-+-+-+-+-+-+-+-+-+
. ^ ^
. | |
. X Y


X is str when is is allocated with str_dup. Y is str when it is freed. Try freeing X instead.
19 Mar, 2013, Rarva.Riendf wrote in the 20th comment:
Votes: 0
Quote
In some situation in C, and I'm not 100% sure which, the compiler won't let you return a non-static string.


May have a warning if activated or in some compilers, but believe me, it can compile real fine :)
0.0/23