18 Jan, 2009, Baiou wrote in the 1st comment:
Votes: 0
Running a modded smaug, I get a crash occasionally when strcpy is called. After some tests, I discovered the problem is buffer overflows but it's the default way smaug had handled these functions. There is no difference between the 1.8 release and the cb I'm working in. Now, I know there are a few ways to fix this such as changing it to sprintf, but thats less efficient and time consuming to go through all of them especially since its the default. Can anyone offer a better way or maybe some insight as to why the default functions aren't working properly? To make it more specific, one of the big issues is in do_help. The mud can output helpfiles, but when a player does do_help, there's a crash due to strcpy. Any and all feedback is welcomed.
18 Jan, 2009, Omega wrote in the 2nd comment:
Votes: 0
Change all sprintf's to snprintf's.

Then change all strcpy's to strncpy, same goes for strcat to strncat.

As they have a variable for max-length.

so strncpy(buf, "Crashy long buffer here", MAX_STRING_LENGTH); <– will never exceed.

Its a long update, but it makes the world of difference.
18 Jan, 2009, Fizban wrote in the 3rd comment:
Votes: 0
Darien said:
Its a long update, but it makes the world of difference.


If you use sed it should take at most 4-5 minutes. Then again there's a reason many people are afraid to play with sed, its syntax isn't the easiest thing in the world to use and can have unexpected reactions.
18 Jan, 2009, Tyche wrote in the 4th comment:
Votes: 0
Unfortunately…
$ cat strncpy.c 
#include <stdio.h>
#define MSL 16
main() {
char a[MSL];
strncpy(a,"Well how do you do my friend?\n",MSL);
printf("%s",a);
}


jlambert@atlas ~
$ gcc strncpy.c ; ./a
Well how do you  !al@
18 Jan, 2009, Baiou wrote in the 5th comment:
Votes: 0
Alright, sounds like a plan. Thank you all.
18 Jan, 2009, Tricky wrote in the 6th comment:
Votes: 0
Tyche said:
Unfortunately…
$ cat strncpy.c 
#include <stdio.h>
#define MSL 16
main() {
char a[MSL];
strncpy(a,"Well how do you do my friend?\n",MSL);
printf("%s",a);
}


jlambert@atlas ~
$ gcc strncpy.c ; ./a
Well how do you  !al@

:biggrin: This is a classic problem given to students learning C.

When assigning arrays for strings, you have to remember to include space for the terminating character '\0'. So in the above example…
char a[MSL];

should be
char a[MSL + 1];

Of course you can not guarantee that everyone will follow this simple rule. Either they never learnt this or they are just lazy. To get around this you could use…
strncpy(buf, "Crashy long buffer here", MAX_STRING_LENGTH - 1);


Tricky
18 Jan, 2009, Mister wrote in the 7th comment:
Votes: 0
Wrong:

No null-character is implicitly appended to the end of the destination, so it will only be null-terminated if the length of the C string in source is less than num. (source)

You need to add the null character at the end by yourself. Better to create your own strncpy-like function, something like:

char * my_strncpy ( char * destination, const char * source, size_t num ) {
strncpy(destinacion, source);
destination[num-1] = '\0';
return destination;
}
18 Jan, 2009, Baiou wrote in the 8th comment:
Votes: 0
Is it safe to do strncpy( buf, whatever, sizeof(buf) );?
18 Jan, 2009, Tricky wrote in the 9th comment:
Votes: 0
My bad for not reading the man page thoroughly. However, allocating an array that is 1 char bigger than your max buffer size should always be done. Then you have space to append the '\0' onto end of the string.

(This is gonna end up like alt.lang.c)

Also just noticed that Tyche's code missed out a #include <string.h>

Tricky
18 Jan, 2009, Tricky wrote in the 10th comment:
Votes: 0
Baiou said:
Is it safe to do strncpy( buf, whatever, sizeof(buf) );?

Yes and no.

Yes you can use that method however as pointed out by Mister you need to make sure the last element of the string is a '\0', you can use Mister's example function code or this macro to make things easier…
#define my_strncpy(dst,src,sz) strncpy(dst, src, sz); (dst)[sz] = '\0';

Of course you would have to change every instance of strncpy to my_strncpy.

Note: My macro assumes that the buffer arrays are 1 char larger than MAX_STRING_LENGTH.

Tricky
18 Jan, 2009, Omega wrote in the 11th comment:
Votes: 0
why not just use mudstrlcpy and mudstrlcat…. (prods afkmud)

Or use the modded versions I released afew years back that fire off wiznets when they would exceed lengths :P

Just a thought.
18 Jan, 2009, Scandum wrote in the 12th comment:
Votes: 0
Accidental string truncation shouldn't happen, so a crash is really the proper response.
18 Jan, 2009, Kline wrote in the 13th comment:
Votes: 0
Scandum said:
Accidental string truncation shouldn't happen, so a crash is really the proper response.


I was told this, too, when I updated to snprintf/strncat. Part of me can agree with the sentiment, yet more of me would rather avoid the crash, notice a malformed string, and then fix it gracefully. Crashes can be very disruptive in a live game – garbled text not as much :).
18 Jan, 2009, Omega wrote in the 14th comment:
Votes: 0
Personally, snprintf and strncat/strncpy, while you may end up with a ugly string, it can prevent crash's. which as stated previously, is not a good thing for a live game. I'd rather take a malformed string any-day.

I can always fix up all my snprintf/strncat/strncpy calls later with a macro that displays a file/function/line of why and where the string is overflowing and being caught, so I can go around and extend my max lengths where applicable.

But thats just me. In anycase, stability comes before pretty appearances.
18 Jan, 2009, quixadhal wrote in the 15th comment:
Votes: 0
If you don't object to using BSD licensed code, you can borrow the source to strlcpy from OpenBSD (or probably FreeBSD as well), which gets around the disconnect between strncat/strncpy and the buffer size not always being NULL terminated.

As for ugly strings… they might not matter if the output is destined for the user's buffer… they might annoy you a whole lot more if they are being formatted to be saved to an area file. :)
19 Jan, 2009, Scandum wrote in the 16th comment:
Votes: 0
Darien said:
Personally, snprintf and strncat/strncpy, while you may end up with a ugly string, it can prevent crash's. which as stated previously, is not a good thing for a live game. I'd rather take a malformed string any-day.

I'll take a crash over corrupted player files any day.
19 Jan, 2009, Kline wrote in the 17th comment:
Votes: 0
Scandum said:
I'll take a crash over corrupted player files any day.


So use a wrapper or do some sanity checks on the result. No crash, no corruption. Win/win?
19 Jan, 2009, Tyche wrote in the 18th comment:
Votes: 0
Here's how we do it.

$ cat strncpy.c 
#define MSL 16
main() {
char a[MSL];
char b[MSL];
char c[MSL];

/* proper use of strncpy */
strncpy(a,"Well how do you do my friend.\n",MSL);
a[MSL-1] = '\0';
printf("%s",a);

/* proper use of strncat */
strncpy(b,"Hello",MSL);
strncat(b,", how do you do my friend.\n",MSL-strlen(b));
b[MSL-1] = '\0';
printf("%s",b);

/* proper use of snprintf */
snprintf(c,MSL,"Well how do you do my friend.\n");
printf("%s",c);


}


jlambert@atlas ~
$ gcc strncpy.c ; ./a
Well how do youHello, how do yWell how do you


I prefer snprintf() because it terminates the string with a NUL.
I would not declare your arrays MAX_STRING_LENGTH+1.
19 Jan, 2009, Scandum wrote in the 19th comment:
Votes: 0
Kline said:
So use a wrapper or do some sanity checks on the result. No crash, no corruption. Win/win?

Sanity should be a given, assuming you're attempting to write a state machine.
19 Jan, 2009, Tyche wrote in the 20th comment:
Votes: 0
Tricky said:
Also just noticed that Tyche's code missed out a #include <string.h>


You're lucky I included stdio.h.
Headers are for people who haven't memorized the function signatures. ;-)
0.0/20