View Issue Details

IDProjectCategoryView StatusLast Update
0000500LDMud 3.3Runtimepublic2018-01-29 22:57
Reporterzesstra Assigned Tolars 
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.3.713 
Fixed in Version3.3.716 
Summary0000500: load_object() may lead to a crash if given an empty string
Descriptionload_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.
TagsNo tags attached.

Relationships

related to 0000525 resolvedzesstra Allow objects with empty names (again) 

Activities

zesstra

2007-02-25 10:41

administrator   ~0000528

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.

2007-02-25 10:42

 

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)
load_object.patch (1,059 bytes)   

2007-02-25 10:45

 

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
  */
find_object.patch (3,136 bytes)   

lars

2007-10-06 22:26

reporter   ~0000554

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.

Issue History

Date Modified Username Field Change
2007-01-30 15:45 zesstra New Issue
2007-02-25 10:41 zesstra Note Added: 0000528
2007-02-25 10:42 zesstra File Added: load_object.patch
2007-02-25 10:45 zesstra File Added: find_object.patch
2007-10-06 22:26 lars Status new => resolved
2007-10-06 22:26 lars Fixed in Version => 3.3.716
2007-10-06 22:26 lars Resolution open => fixed
2007-10-06 22:26 lars Assigned To => lars
2007-10-06 22:26 lars Note Added: 0000554
2009-01-08 06:03 zesstra Relationship added related to 0000525
2010-11-16 10:42 lars Source_changeset_attached => ldmud.git master 4766786b
2018-01-29 19:59 lars Source_changeset_attached => ldmud.git master 4766786b
2018-01-29 22:57 lars Source_changeset_attached => ldmud.git master 4766786b