Welcome to the Builder Academy

Question MAX_NAME_LENGTH crash, repeatable, all versions

More
10 Feb 2020 23:11 #8563 by thomas
I think the +1 fix is a hack. It would have to be done everywhere we allocate a buffer for a name.
Instead, we must limit to 19 chars, using <= instead of <. That is consistent with the buffer size.
The following user(s) said Thank You: Salty

Please Log in or Create an account to join the conversation.

More
11 Feb 2020 01:15 #8564 by Castillo
Code:
#define MAX_NAME_LENGTH 20 /**< Max PC/NPC name length */ #define MAX_PWD_LENGTH 30 /**< Max PC password length */

At 31 chars, a password is illegal. 30 chars password is accepted. It's really the MAX_PWD_LENGTH

If the name is restricted to 19, shouldn't MAX_NAME_LENGTH renamed somethings like NAME_BUFSIZE?
That way we know 20 is 19 chars +including \0 ?

Or, is there any reason the particuliar buffer that crash. Couldn't be a pointer?
and, instead of using strcpy, use strdup?

Just some thinking...

Please Log in or Create an account to join the conversation.

More
11 Feb 2020 01:28 #8565 by Salty

If the name is restricted to 19, shouldn't MAX_NAME_LENGTH renamed somethings like NAME_BUFSIZE?
That way we know 20 is 19 chars +including \0 ?

Or, is there any reason the particuliar buffer that crash. Couldn't be a pointer?
and, instead of using strcpy, use strdup?


Strnlen() vs strlen()

Probably just better to use strnlen() with n=MAX_NAME_LENGTH instead of strlen()? This way we are guaranteed to get our null byte.

Please Log in or Create an account to join the conversation.

More
11 Feb 2020 14:26 - 11 Feb 2020 15:04 #8566 by krell

Salty wrote:

If the name is restricted to 19, shouldn't MAX_NAME_LENGTH renamed somethings like NAME_BUFSIZE?
That way we know 20 is 19 chars +including \0 ?
<snipped>


<snipped>

Probably just better to use strnlen() with n=MAX_NAME_LENGTH instead of strlen()? This way we are guaranteed to get our null byte.


I guess it comes down to whether null is added or not. If the buffer holding the string is too full to take a terminating NUL then that can cause instability later on when that string needs to be parsed, is that right? While I'm personally leaning towards strnlen(), I think using strlen() and >= or MAX_NAME_LENGTH + 1 should functionally do the same as strlen() counts up to the terminating NUL.

Addendum: I don't think strnlen() guarantees a terminating NUL as, according to the man pages,

The strnlen() function computes the length of the string s, up to maxlen
characters. The strnlen() function will never attempt to address more
than maxlen characters, making it suitable for use with character arrays
that are not guaranteed to be NUL-terminated.


Which would be useful for checking for such strings and adding null to the end to fix them, I guess.
Last edit: 11 Feb 2020 15:04 by krell.

Please Log in or Create an account to join the conversation.

More
12 Feb 2020 20:51 #8567 by Castillo

I guess it comes down to whether null is added or not. If the buffer holding the string is too full to take a terminating NUL then that can cause instability later on when that string needs to be parsed, is that right? While I'm personally leaning towards strnlen(), I think using strlen() and >= or MAX_NAME_LENGTH + 1 should functionally do the same as strlen() counts up to the terminating NUL.

Agreed.

I think the +1 fix is a hack. It would have to be done everywhere we allocate a buffer for a name.
Instead, we must limit to 19 chars, using <= instead of <. That is consistent with the buffer size.

I just wanted to explain why i choosed +1 instead of >=.
I don't mind loosing 1 char. I understand why you don't like it.

What bother me is confusion if sometimes
MAX_SOMETHING1_LENGTH = maxlen + \0
(e.g MAX_PWD_LENGTH = 30 means 30 chars + \0)

and sometimes

MAX_SOMETHING2_LENGTH = maxlen - 1 + \0
(e.g MAX_NAME_LENGTH = 20 means 19 chars +\0)

Bob

Please Log in or Create an account to join the conversation.

More
22 Feb 2020 02:43 - 22 Feb 2020 20:34 #8574 by doggo
The issue is that a char * is being casually converted to a char array when it is passed to strcpy.

Having absolutely nothing to do with the gcc flags that tba-2020 uses, the following code (which is an example based on the test that Castillo did above) ought to crash!
Code:
#include <stdio.h> #include <string.h> #include <stdlib.h> int main() { /* creates a variable chname, which is a pointer * to a character, and room for 6 characters (when * the NULL terminator is accounted for) is reserved */ char *chname = "doggo"; /* creates a character array which has room for 5 characters */ char name[5]; /* will crash, because 6 characters are being * written to an array which only has room for 5 */ strcpy(name, chname); return 0; }
The fact that you didn't have issues before incorporating the compiler flags
Code:
-g -O2 -Wall -Wno-char-subscripts -Wno-unused-but-set-variable
is actually the surprising part. It should crash because it's trying to modify memory it doesn't have access to.

To respond to Thomas' thoughts:

I think the +1 fix is a hack. It would have to be done everywhere we allocate a buffer for a name.
Instead, we must limit to 19 chars, using <= instead of <. That is consistent with the buffer size.

This is true, and is to be expected! It's no less weird to implement
Code:
#define MAX_NAME_LENGTH 20
and then insist that names be no more than 19 characters long.

The solution suggested by Castillo:
Code:
/** structure for list of recent players (see 'recent' command) */ struct recent_player { int vnum; /* The ID number for this instance */ - char name[MAX_NAME_LENGTH]; /* The char name of the player */ + char name[MAX_NAME_LENGTH + 1]; /* The char name of the player */ bool new_player; /* Is this a new player? */ bool copyover_player; /* Is this a player that was on during the last copyover? */ time_t time; /* login time */ char host[HOST_LENGTH+1]; /* Host IP address */ struct recent_player *next; /* Pointer to the next instance */ };
is not a hack. In fact it is the natural choice here.
Last edit: 22 Feb 2020 20:34 by doggo. Reason: formatting

Please Log in or Create an account to join the conversation.

Time to create page: 0.203 seconds