View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000500 | LDMud 3.3 | Runtime | public | 2007-01-30 14:45 | 2018-01-29 21:57 |
Reporter | zesstra | Assigned To | |||
Priority | normal | Severity | crash | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.3.713 | ||||
Fixed in Version | 3.3.716 | ||||
Summary | 0000500: load_object() may lead to a crash if given an empty string | ||||
Description | load_object() and lookfor_object() do not check, if the received string is an empty string. They won't find a corresponding object and eventually load it via load_object(). If there is a file /.c in the mudlib, the file will be found and compiled. (Because the driver implicitly adds .c to the name.) Then load_object() assigns the empty string to ob->name and tries to enter the object in the object hash table. enter_object_hash looks up the object in the hash table first and will always find some because of the empty name. In debug mode that results in calling fatal("Duplicate object in objetct hash table"). load_object() should therefore check and refuse loading if the given name is empty. I have a stacktrace and core file availabe if someone is interested to check personally. | ||||
Tags | No tags attached. | ||||
Attached Files | load_object.patch (1,059 bytes)
diff -ur ldmud-3.3.714/src/simulate.c ldmud-3.3.714-zesstra/src/simulate.c --- ldmud-3.3.714/src/simulate.c 2006-07-10 04:42:06.000000000 +0200 +++ ldmud-3.3.714-zesstra/src/simulate.c 2007-02-25 16:15:09.000000000 +0100 @@ -1762,6 +1762,13 @@ if ('/' == lname[0]) fatal("Improper filename '%s' passed to load_object()\n", lname); #endif + + // empty lnames may lead to a driver crash in enter_object_hash() if there + // exists a file '.c' in the root of the mudlib. + if ((name_length=strlen(lname)) <= 1) { + load_object_error("Illegal file to load (empty filename)", lname, chain); + /* NOTREACHED */ + } /* It could be that the passed filename is one of an already loaded * object. In that case, simply return that object. @@ -1776,7 +1783,6 @@ * the second because lname might be a buffer which is deleted * during the compilation process. */ - name_length = strlen(lname); name = alloca(name_length+2); fname = alloca(name_length+4); if (!name || !fname) find_object.patch (3,136 bytes)
diff -ur ldmud-3.3.714/src/mstrings.c ldmud-3.3.714-zesstra/src/mstrings.c --- ldmud-3.3.714/src/mstrings.c 2006-07-10 04:43:25.000000000 +0200 +++ ldmud-3.3.714-zesstra/src/mstrings.c 2007-02-25 15:56:17.000000000 +0100 @@ -179,22 +179,16 @@ #endif /* EXT_STRING_STATS */ /*-------------------------------------------------------------------------*/ -static INLINE whash_t +/* +whash_t hash_string (const char * const s, size_t size) - -/* Compute the hash for string <s> of length <size> and return it. + * Compute the hash for string <s> of length <size> and return it. * The result will always be non-zero. + * NOTE: The definition of the function is now in mstrings.h because it is + * also used in otable.c and the definition needs to be in every compilation + * unit which uses it. */ -{ - whash_t hash; - - hash = whashmem(s, size, MSTRING_HASH_LENGTH); - if (!hash) - hash = 1 << (sizeof (hash) * CHAR_BIT - 1); - return hash; -} /* hash_string() */ - /*-------------------------------------------------------------------------*/ static INLINE whash_t get_hash (string_t * pStr) diff -ur ldmud-3.3.714/src/mstrings.h ldmud-3.3.714-zesstra/src/mstrings.h --- ldmud-3.3.714/src/mstrings.h 2006-07-10 04:43:26.000000000 +0200 +++ ldmud-3.3.714-zesstra/src/mstrings.h 2007-02-25 15:52:32.000000000 +0100 @@ -117,6 +117,26 @@ /* --- Inline functions and macros --- */ +/*-------------------------------------------------------------------------*/ +static INLINE whash_t +hash_string (const char * const s, size_t size) +/* Compute the hash for string <s> of length <size> and return it. + * The result will always be non-zero. + * Needs to be in mstrings.h to be inlined because otable.c also uses it. + * NOTE: it is static so the compiler generates inline functions only visible + * in the compilation unit. Thus the linker does not bail out in case the + * compiler does not inline it and it is portable on systems not supporting inline. + */ +{ + whash_t hash; + + hash = whashmem(s, size, MSTRING_HASH_LENGTH); + if (!hash) + hash = 1 << (sizeof (hash) * CHAR_BIT - 1); + return hash; +} /* hash_string() */ +/*-------------------------------------------------------------------------*/ + #define mstr_mem_size(s) \ (sizeof(string_t) + (s)->size) diff -ur ldmud-3.3.714/src/otable.c ldmud-3.3.714-zesstra/src/otable.c --- ldmud-3.3.714/src/otable.c 2006-07-10 04:42:56.000000000 +0200 +++ ldmud-3.3.714-zesstra/src/otable.c 2007-02-25 16:12:12.000000000 +0100 @@ -51,10 +51,10 @@ #if !( (OTABLE_SIZE) & (OTABLE_SIZE)-1 ) # define ObjHash(s) (mstr_get_hash(s) & ((OTABLE_SIZE)-1) ) -# define ObjHashStr(s,len) (whashmem(s, len, MSTRING_HASH_LENGTH) & ((OTABLE_SIZE)-1) ) +# define ObjHashStr(s,len) (hash_string(s, len) & ((OTABLE_SIZE)-1) ) #else # define ObjHash(s) (mstr_get_hash(s) % OTABLE_SIZE) -# define ObjHashStr(s,len) (whashmem(s, len, MSTRING_HASH_LENGTH) % OTABLE_SIZE) +# define ObjHashStr(s,len) (hash_string(s, len) % OTABLE_SIZE) #endif /* Hash the string <s> and compute the appropriate table index */ | ||||
|
I finally found some time to dive deeper into the problem. It is actually a little bit different from what I originally thought. load_object("") loads the /.c correctly and enters it into the hash table. Unfortunately, lookfor_object() and load_object() cannot find objects with an empty name, thus causing load_object() to load it a second time, upon which it crashes. lookfor_object() and load_object() both use lookup_object_hash_str() for looking up existing objects. That function uses find_obj_n_str() which calculates the hash by a direct call to whashmem(), which returns 0 as hash for an empty C-string. Thus the object is not found. In contrast, enter_object_hash() uses find_obj_n() which finally ends in hash_string() in mstrings.c, which does the hash computation by call to whashmem() in a similar way, but ensures to return a non-zero hash to the caller. Thus, even the object with the empty name is found. There are now 2 possibilities: 1) define "" as illegal object/file name and make load_obect() refuse to load it. I will attach a patch called load_object.patch for that. 2) change find_obj_n_str() to find the object with name "", by using hash_string() from mstrings.c instead of calling whashmem() directly. hash_string() in mstrings.c is an inline function, therefore it is neccessary to move it to mstrings.h if it should remain suitable for inlining. See find_object.patch for that. Both solutions work fine, which to choose remains to you. ;-) If there is some chance, that an object named "" may cause havoc somewhere else, I would prefer solution 1, otherwise 2 may be an option. |
|
I liked both solutions, for their own different reasons. Refusing to load objects with an empty name is a sane thing to do. And the object table should use exactly the same hash regardless of whether it hashes a given mstring, or a regular C-String. I did not make the hash_string() function inline for the object table, though - the expensive part is whashmem() anyway. |
Date Modified | Username | Field | Change |
---|---|---|---|
2007-01-30 14:45 | zesstra | New Issue | |
2007-02-25 09:41 | zesstra | Note Added: 0000528 | |
2007-02-25 09:42 | zesstra | File Added: load_object.patch | |
2007-02-25 09:45 | zesstra | File Added: find_object.patch | |
2007-10-06 20:26 |
|
Status | new => resolved |
2007-10-06 20:26 |
|
Fixed in Version | => 3.3.716 |
2007-10-06 20:26 |
|
Resolution | open => fixed |
2007-10-06 20:26 |
|
Assigned To | => lars |
2007-10-06 20:26 |
|
Note Added: 0000554 | |
2009-01-08 05:03 | zesstra | Relationship added | related to 0000525 |
2010-11-16 09:42 |
|
Source_changeset_attached | => ldmud.git master 4766786b |
2018-01-29 18:59 |
|
Source_changeset_attached | => ldmud.git master 4766786b |
2018-01-29 21:57 |
|
Source_changeset_attached | => ldmud.git master 4766786b |