Malfunction of gcry_sexp_car

[Benjamin Pousse]

Hello,

(This is my first contribution. So I apologize if this is not the right
way to contribute.)

The function gcry_sexp_car seems buggy to me (tested on libgcrypt
version 1.4.6, 1.5.0, 1.6.0-git6078b05): it doesn't return any result on
S-expression starting with a data element.
For example, applied to the S-expression (hello "123"), it returns an
empty S-expression (I expect the S-expression (hello)).

In fact, after some "investigation", the function gcry_sexp_nth appears
unable to return the n-th element of a S-expression when this element is
not a list.
More precisely, in the function gcry_sexp_nth, the code in the first
"if" after the first loop "while" does not construct a valid gcry_sexp.

Please find at the end of this mail my patch to solve this problem (the
patch is large because I apply a regular indentation on the full
function).

Regards,
Benjamin.


diff --git a/src/sexp.c b/src/sexp.c
index 0877773..12b57ba 100644
--- a/src/sexp.c
+++ b/src/sexp.c
@@ -549,73 +549,96 @@ gcry_sexp_nth( const gcry_sexp_t list, int number
)
     int level = 0;

     if ( !list || list->d[0] != ST_OPEN )
- return NULL;
+      return NULL;
     p = list->d;

-    while ( number > 0 ) {
- p++;
- if ( *p == ST_DATA ) {
-     memcpy ( &n, ++p, sizeof n );
-     p += sizeof n + n;
-     p--;
-     if ( !level )
- number--;
- }
- else if ( *p == ST_OPEN ) {
-     level++;
- }
- else if ( *p == ST_CLOSE ) {
-     level--;
-     if ( !level )
- number--;
- }
- else if ( *p == ST_STOP ) {
-     return NULL;
- }
-    }
+    while ( number > 0 )
+      {
+        p++;
+        if ( *p == ST_DATA )
+          {
+         memcpy ( &n, ++p, sizeof n );
+         p += sizeof n + n;
+         p--;
+         if ( !level )
+           number--;
+       }
+        else if ( *p == ST_OPEN )
+          {
+            level++;
+          }
+        else if ( *p == ST_CLOSE )
+       {
+            level--;
+            if ( !level )
+              number--;
+          }
+        else if ( *p == ST_STOP )
+          {
+            return NULL;
+          }
+      }
     p++;

-    if ( *p == ST_DATA ) {
- memcpy ( &n, p, sizeof n ); p += sizeof n;
- newlist = gcry_malloc ( sizeof *newlist + n + 1 );
+    if ( *p == ST_DATA )
+      {
+        memcpy ( &n, p + 1, sizeof n );
+        /* Allocate 1 (= sizeof *newlist) byte for ST_OPEN
+                    1 byte for ST_DATA
+                    sizeof n byte for n
+                    n byte for the data
+                    1 byte for ST_CLOSE
+                    1 byte for ST_STOP*/
+        newlist = gcry_malloc ( sizeof *newlist + 1 + sizeof n + n + 2
);
         if (!newlist)
           return NULL;
- d = newlist->d;
- memcpy ( d, p, n ); d += n;
- *d++ = ST_STOP;
-    }
-    else if ( *p == ST_OPEN ) {
- const byte *head = p;
-
- level = 1;
- do {
-     p++;
-     if ( *p == ST_DATA ) {
- memcpy ( &n, ++p, sizeof n );
- p += sizeof n + n;
- p--;
-     }
-     else if ( *p == ST_OPEN ) {
- level++;
-     }
-     else if ( *p == ST_CLOSE ) {
- level--;
-     }
-     else if ( *p == ST_STOP ) {
- BUG ();
-     }
- } while ( level );
- n = p + 1 - head;
+        d = newlist->d;
+        *d = ST_OPEN;
+        d++;
+        memcpy ( d, p, 1 + sizeof n + n );  /* Copy ST_DATA, n and the
data from p to d*/
+        d += 1 + sizeof n + n;
+        *d = ST_CLOSE;
+        d++;
+        *d = ST_STOP;
+      }
+    else if ( *p == ST_OPEN )
+      {
+        const byte *head = p;
+
+        level = 1;
+        do
+          {
+            p++;
+            if ( *p == ST_DATA )
+              {
+                memcpy ( &n, ++p, sizeof n );
+                p += sizeof n + n;
+                p--;
+              }
+            else if ( *p == ST_OPEN )
+              {
+                level++;
+              }
+            else if ( *p == ST_CLOSE )
+              {
+                level--;
+              }
+            else if ( *p == ST_STOP )
+              {
+                BUG ();
+              }
+          } while ( level );
+        n = p + 1 - head;

- newlist = gcry_malloc ( sizeof *newlist + n );
+        newlist = gcry_malloc ( sizeof *newlist + n );
         if (!newlist)
           return NULL;
- d = newlist->d;
- memcpy ( d, head, n ); d += n;
- *d++ = ST_STOP;
-    }
+        d = newlist->d;
+        memcpy ( d, head, n ); d += n;
+        *d++ = ST_STOP;
+      }
     else
- newlist = NULL;
+      newlist = NULL;

     return normalize (newlist);
}