[Nmh-workers] bug in m_getfld.c

[Peter Maydell]

I was investigating debian bug 359167 (a report of inc dumping core), and
tracked it down to a bug (actually two very similar bugs) in m_getfld.c.
Since that's such a nasty piece of code I thought I'd post the patch here
in case anybody wanted to peer-review it.

The bugs are in the bit of the code in the BODY case which deals with
checking for a partial message delimiter at the end of the stdio buffer.
Bug one is that there's nothing stopping the code from walking off the
front of the stdio buffer if it is smaller than the prefix we're checking.
(This is what caused the bug reporter's segfault. Very dependent on message
contents and stdio buffer length.)
Bug two is that if the stdio buffer contains a NUL byte in the wrong place
then we can walk off the beginning of the prefix string buffer.

I believe this patch to be correct, but I haven't tested it much.
Don't suppose anybody has a nice pile of test mailbox files? :-)

-- PMM

===begin patch===
Index: sbr/m_getfld.c
===================================================================
RCS file: /sources/nmh/nmh/sbr/m_getfld.c,v
retrieving revision 1.13
diff -u -r1.13 m_getfld.c
--- sbr/m_getfld.c      5 Apr 2008 18:41:37 -0000       1.13
+++ sbr/m_getfld.c      8 Aug 2008 15:50:04 -0000
@@ -466,22 +466,35 @@
                    ep = bp + c - 1;
                    if ((sp = pat_map[*ep])) {
                        do {
-                           cp = sp;
-                           while (*--ep == *--cp)
-                           ;
-                           if (cp < fdelim) {
-                               if (ep >= bp)
-                                   /*
-                                    * ep < bp means that all the buffer
-                                    * contains is a prefix of delim.
-                                    * If this prefix is really a delim, the
-                                    * m_eom call at entry should have found
-                                    * it.  Thus it's not a delim and we can
-                                    * take all of it.
+                           /* This if() is true unless (a) the buffer is too
+                            * small to contain this delimiter prefix, or
+                            * (b) it contains exactly enough chars for the
+                            * delimiter prefix.
+                            * For case (a) obviously we aren't going to match.
+                            * For case (b), if the buffer really contained 
exactly
+                            * a delim prefix, then the m_eom call at entry
+                            * should have found it.  Thus it's not a delim
+                            * and we know we won't get a match.
+                            */
+                           if (((sp - fdelim) + 2) <= c) {
+                               cp = sp;
+                               /* Unfortunately although fdelim has a 
preceding NUL
+                                * we can't use this as a sentinel in case the 
buffer
+                                * contains a NUL in exactly the wrong place 
(this
+                                * would cause us to run off the front of 
fdelim).
+                                */
+                               while (*--ep == *--cp)
+                                   if (cp < fdelim)
+                                       break;
+                               if (cp < fdelim) {
+                                   /* we matched the entire delim prefix,
+                                    * so only take the buffer up to there.
+                                    * we know ep >= bp -- check above prevents 
underrun
                                     */
                                    c = (ep - bp) + 2;
-                           break;
-                       }
+                                   break;
+                               }
+                           }
                            /* try matching one less char of delim string */
                            ep = bp + c - 1;
                        } while (--sp > fdelim);
===endit===