[Sks-devel] Re: zero-length MPIs (was: Re: mpi error with check-trustdb in 1.4.2 - resolved)

[David Shaw]

On Wed, Aug 24, 2005 at 03:07:17PM +0200, Klaus Singvogel wrote:

> I noticed that these messages are coming from
> mpi/mpicoder.c:mpi_read() and had a closer look at it. :-)
> 
> The second if check, for "goto overflow;" seems a bit doubtful (maybe
> a copy&paste without to much thinking whats coming next ? :-) As
> there are no mandatory reads from the iobuf coming, only optional
> reads, I changed the code to "if (++nread > nmax)" and the problem
> was gone (see attached patch).

I think this is very close, but not perfect.  You must also protect
against accidentally reading too many bytes, as then you can't parse
the rest of the stream.

Try this patch.

David
-------------- next part --------------
Index: mpicoder.c
===================================================================
--- mpicoder.c	(revision 3878)
+++ mpicoder.c	(working copy)
@@ -80,16 +80,20 @@
     mpi_limb_t a;
     MPI val = MPI_NULL;
 
+    if (nread == nmax)
+        goto overflow;
     if( (c = iobuf_get(inp)) == -1 )
 	goto leave;
-    if (++nread >= nmax)
+    nread++;
+    nbits = c << 8;
+
+    if (nread == nmax)
         goto overflow;
-    nbits = c << 8;
     if( (c = iobuf_get(inp)) == -1 )
 	goto leave;
-    if (++nread >= nmax)
-        goto overflow;
+    nread++;
     nbits |= c;
+
     if( nbits > MAX_EXTERN_MPI_BITS ) {
 	log_error("mpi too large for this implementation (%u bits)\n", nbits);
 	goto leave;
@@ -112,7 +116,7 @@
     for( ; j > 0; j-- ) {
 	a = 0;
 	for(; i < BYTES_PER_MPI_LIMB; i++ ) {
-            if (nread >= nmax) {
+            if (nread == nmax) {
 #ifdef M_DEBUG
                 mpi_debug_free (val);
 #else