Invalid INTEGER values are saved to database storages in x86_64

[avillacis]

In 64-bit architectures (seen with x86_64), where sizeof(void _) is larger than sizeof(int), the recovery of os_type_BOOLEAN and os_type_INTEGER types through os_object_iter_get() assign sizeof(int) bytes pointed to by the parameter void *_val. However, since the pointed to variable is sizeof(void *), this results in the high bytes of the variable not being set and therefore left with garbage values. In the database storage drivers, the values are then inspected as a void * cast to long int, or plain long, which access the bogus upper bytes of the value. The final effect is that the database driver randomly stores a very large integer value to the database (if saving an os_type_INTEGER), or else end up storing a TRUE value when the higher code meant a FALSE value (if saving an os_type_BOOLEAN).

Issue #48 is an instance of this bug, but the fix was committed only for the postgresql driver, and only for the os_type_INTEGER type. The mysql and sqlite drivers are still affected, as well as os_type_BOOLEAN for postgresql.

[avillacis]

I have the following patch that fixes the issues (applied to 2.3.2):

diff -ur jabberd-2.3.2-bak/storage/storage_mysql.c jabberd-2.3.2/storage/storage_mysql.c
--- jabberd-2.3.2-bak/storage/storage_mysql.c   2014-07-23 09:57:17.603740035 -0500
+++ jabberd-2.3.2/storage/storage_mysql.c   2014-07-23 11:26:16.840737235 -0500
@@ -194,16 +194,24 @@
             o = os_iter_object(os);
             if(os_object_iter_first(o))
                 do {
+                    /* For os_type_BOOLEAN and os_type_INTEGER, sizeof(int) bytes
+                       are stored in val, which might be less than sizeof(void *).
+                       Therefore, the difference is garbage unless cleared first.
+                     */
+                    val = NULL;
                     os_object_iter_get(o, &key, &val, &ot);

                     switch(ot) {
                         case os_type_BOOLEAN:
-                            cval = val ? strdup("1") : strdup("0");
+                            /* Only sizeof(int) bytes must be checked for value */
+                            cval = *((int *)(&val)) ? strdup("1") : strdup("0");
                             break;

                         case os_type_INTEGER:
                             cval = (char *) malloc(sizeof(char) * 20);
-                            sprintf(cval, "%ld", (long int) val);
+
+                            /* Only sizeof(int) bytes must be checked for value */
+                            sprintf(cval, "%d", *((int *)(&val)));
                             break;

                         case os_type_STRING:
@@ -218,7 +226,7 @@
                             strncpy(cval, "NAD", 3);
                             break;

-           case os_type_UNKNOWN:
+                        case os_type_UNKNOWN:
                             break;
                     }

diff -ur jabberd-2.3.2-bak/storage/storage_oracle.c jabberd-2.3.2/storage/storage_oracle.c
--- jabberd-2.3.2-bak/storage/storage_oracle.c  2014-07-23 09:57:17.609740036 -0500
+++ jabberd-2.3.2/storage/storage_oracle.c  2014-07-23 11:38:38.416736848 -0500
@@ -388,18 +388,26 @@
       {
         do 
         {
+          /* For os_type_BOOLEAN and os_type_INTEGER, sizeof(int) bytes
+             are stored in val, which might be less than sizeof(dvoid *).
+             Therefore, the difference is garbage unless cleared first.
+           */
+          val = NULL;
           os_object_iter_get(o, &key, &val, &ot);

           switch(ot) 
           {
             case os_type_BOOLEAN:
-              cval = val ? strdup("1") : strdup("0");
+              /* Only sizeof(int) bytes must be checked for value */
+              cval = *((int *)(&val)) ? strdup("1") : strdup("0");
               vlen = 1;
               break;

             case os_type_INTEGER:
               cval = (char *) malloc(sizeof(char) * 20);
-              sprintf(cval, "%d", (int) val);
+              
+              /* Only sizeof(int) bytes must be checked for value */
+              sprintf(cval, "%d", *((int *)(&val)));
               vlen = strlen(cval);
               break;

diff -ur jabberd-2.3.2-bak/storage/storage_pgsql.c jabberd-2.3.2/storage/storage_pgsql.c
--- jabberd-2.3.2-bak/storage/storage_pgsql.c   2014-07-23 09:57:17.607740036 -0500
+++ jabberd-2.3.2/storage/storage_pgsql.c   2014-07-23 11:26:18.734737234 -0500
@@ -194,16 +194,24 @@
             o = os_iter_object(os);
             if(os_object_iter_first(o))
                 do {
+                    /* For os_type_BOOLEAN and os_type_INTEGER, sizeof(int) bytes
+                       are stored in val, which might be less than sizeof(void *).
+                       Therefore, the difference is garbage unless cleared first.
+                     */
+                    val = NULL;
                     os_object_iter_get(o, &key, &val, &ot);

                     switch(ot) {
                         case os_type_BOOLEAN:
-                            cval = val ? strdup("t") : strdup("f");
+                            /* Only sizeof(int) bytes must be checked for value */
+                            cval = *((int *)(&val)) ? strdup("t") : strdup("f");
                             break;

                         case os_type_INTEGER:
                             cval = (char *) malloc(sizeof(char) * 20);
-                            sprintf(cval, "%ld", (int) (intptr_t) val);
+
+                            /* Only sizeof(int) bytes must be checked for value */
+                            sprintf(cval, "%d", *((int *)(&val)));
                             break;

                         case os_type_STRING:
diff -ur jabberd-2.3.2-bak/storage/storage_sqlite.c jabberd-2.3.2/storage/storage_sqlite.c
--- jabberd-2.3.2-bak/storage/storage_sqlite.c  2014-07-23 09:57:17.602740035 -0500
+++ jabberd-2.3.2/storage/storage_sqlite.c  2014-07-23 11:41:02.303736773 -0500
@@ -292,15 +292,22 @@
        o = os_iter_object (os);
        if (os_object_iter_first(o))
        do {
+           /* For os_type_BOOLEAN and os_type_INTEGER, sizeof(int) bytes
+              are stored in val, which might be less than sizeof(void *).
+              Therefore, the difference is garbage unless cleared first.
+            */
+           val = NULL;
            os_object_iter_get (o, &key, &val, &ot);

            switch(ot) {
             case os_type_BOOLEAN:
-             sqlite3_bind_int (stmt, i + 2, val ? 1 : 0);
+             /* Only sizeof(int) bytes must be checked for value */
+             sqlite3_bind_int (stmt, i + 2, *((int *)(&val)) ? 1 : 0);
              break;

             case os_type_INTEGER:
-             sqlite3_bind_int (stmt, i + 2, (long)val); // HACK ugly hack for pointer-to-int-cast
+             /* Only sizeof(int) bytes must be checked for value */
+             sqlite3_bind_int (stmt, i + 2, *((int *)(&val)));
              break;

             case os_type_STRING:

[sdebnath]

Just saw this bug after filing #82. A simpler and cleaner fix for the bool could be (storage_pgsql.c for BOOLEAN):
- cval = val ? strdup("t") : strdup("f");
+ cval = ((int)val == 1) ? strdup("t") : strdup("f");

You should fork the repo, create a branch, apply the fixes and create a pull request. I can test PGSQL for you.

[smokku]

I agree that plain casting is way simpler that pointer magick (and casting is what effectively this pointer magic does). Just one comment though:
C standard (and most other languages after that) define false as 0 (zero) and true as anything not false. Thus assuming true == 1 smells. ( http://c2.com/cgi/wiki?CodeSmell )
Correct test for truthiness is (val != 0).

[sdebnath]

Agree with val != 0, see pull request #84

[smokku]

Fixed by pull #84