Rebuild CGIFormData on Parser/StreamReader, fix a 2-byte-lookahead bug

CGIFormData now inherits Parser instead of MString and parses a CGI
request body (urlencoded or multipart/form-data) resumably against the
shared StreamReader, exposing the result via operator[] - matching every
other Parser in this redesign. multipart/form-data reuses IMFBoundary and
IMFHeader via checkParse() (chained through the same StreamReader) for
each part's boundary line and header block; urlencoded bodies wait for
the full Content-Length before delegating to the existing
CGIFormattedData parser. File-upload parts (Content-Disposition with a
filename= parameter) are recognized and skipped, matching this
codebase's convention that uploads get a separate mechanism.

Values are stored under an indexed "name:N" key (N = 0-based occurrence
count for that name), matching the :N-suffix convention JetCore's
Tag::getVariable already applies to its $[:name] CGI variable syntax
(defaulting to :0) - this also naturally supports repeated field names
(checkboxes, multi-select). operator[] applies the same :0 default so a
bare name still works when called directly, not just through that
convention layer.

Two real resumability bugs found and fixed via a byte-at-a-time test
(feeding one byte per StreamReader::feed() call, the harshest possible
fragmentation):
- CGIFormData's opening boundary marker ("--boundary", no leading CRLF
  per RFC 2046 for the first boundary) crammed "find+consume the marker"
  and "disambiguate -- vs CRLF" into one state; when the disambiguation
  needed more data, INCOMPLETE was returned without recording that the
  marker was already consumed, so retrying re-searched for it past where
  it had already been consumed, silently skipping content forever. Split
  into two states (INIT / OPENING_TAIL), the same fix already applied
  once to IMFBoundary's PREFIX/TAIL split.
- IMFHeader's FIELD state checked eod() (a 1-byte availability check)
  immediately before an ifNext("\r\n") call that needs 2 bytes - a lone
  trailing \r with no \n yet available was wrongly treated as malformed
  instead of "wait for more data". Fixed to check getLength() < 2.

Verified via a dedicated test (64 assertions total across this and the
earlier Parser/StreamReader test) covering urlencoded, multipart with a
skipped file upload, and the byte-at-a-time stress case - plus a real
end-to-end run through jet-2.0 with a simulated CGI multipart POST
request (see the matching JetCore commit).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

CGIFormData.cpp

@@ -1,58 +1,168 @@
 #include "CGIFormData.h"
-#include "malloc.h"
+#include "CGIFormattedData.h"
+#include "Exception.h"
 
 namespace coreutils {
 
-   CGIFormData::CGIFormData(int fd, ZString contentType, int contentLength, ZString boundary)
-     : contentType(contentType) {
-      if(contentType.startsWith("application/x-www-form-urlencoded")) {
-	 read(fd, contentLength);
-	 formdata = new CGIFormattedData(*this);
-      } else if(contentType.startsWith("multipart/form-data")) {
-	 read(fd, contentLength);
-	 formdata = new CGIFormattedData();
+   CGIFormData::CGIFormData(StreamReader &reader, ZString contentType, int contentLength, ZString boundary)
+     : Parser(reader), contentType(contentType), contentLength(contentLength), boundary(boundary) {
+      if (contentType.startsWith("application/x-www-form-urlencoded"))
+         mode = URLENCODED;
+      else if (contentType.startsWith("multipart/form-data"))
+         mode = MULTIPART;
+      else
+         mode = UNKNOWN;
+   }
 
-	 MString delimiter;
-	 delimiter << "--" << boundary;
-	 ZString body(*this);
-	 body.split(delimiter);
+   CGIFormData::~CGIFormData() {
+      delete headerParser;
+      delete boundaryParser;
+   }
 
-	 for(int ix = 0; ix < body.getList().size(); ++ix) {
-	    ZString part = body[ix];
-	    if(part.getLength() == 0 || part.startsWith("--"))
-	      continue;
+   void CGIFormData::extractPartName(IMFHeader &header) {
+      auto &fields = header.getHeaders();
+      MString disposition = fields[MString("Content-Disposition")];
+      ZString dz(disposition.getData(), disposition.getLength());
 
-	    int headerEnd = part.find("\r\n\r\n");
-	    if(headerEnd == -1)
-	      continue;
+      currentSkip = (dz.find(ZString("filename=\"")) != -1);
 
-	    ZString headers = part.substring(0, headerEnd);
-	    if(headers.find("filename=\"") != -1)
-	      // file uploads are out of scope here; handled via a separate upload mechanism.
-	      continue;
-
-	    int namePos = headers.find("name=\"");
-	    if(namePos == -1)
-	      continue;
-
-	    ZString afterName = headers.substring(namePos + 6);
-	    int nameEnd = afterName.find("\"");
-	    if(nameEnd == -1)
-	      continue;
-	    ZString name = afterName.substring(0, nameEnd);
-
-	    ZString value = part.substring(headerEnd + 4);
-	    value.trimCRLF();
-
-	    formdata->data[name] = value;
-	 }
+      int namePos = dz.find(ZString("name=\""));
+      if (namePos != -1) {
+         ZString afterName = dz.substring(namePos + 6);
+         int nameEnd = afterName.find(ZString("\""));
+         if (nameEnd != -1)
+            currentName = MString(afterName.substring(0, nameEnd));
       }
    }
 
+   Parser::Status CGIFormData::parse() {
+      MString &buffer = reader.getBuffer();
+
+      if (mode == UNKNOWN) {
+         state = DONE;
+         return Status::COMPLETE;
+      }
+
+      switch (state) {
+
+       case INIT:
+          if (mode == URLENCODED) {
+             state = URLENCODED_BODY;
+             return Status::DATA_AVAIL;
+          }
+          // MULTIPART: the very first boundary line has no leading CRLF
+          // (RFC 2046) - handled here directly rather than via IMFBoundary,
+          // which always expects the CRLF that terminates a PRECEDING part.
+          // Split into two states (find+consume the marker, then separately
+          // disambiguate -- vs CRLF) so that if the marker is consumed but
+          // there isn't yet enough data to tell -- from CRLF, retrying does
+          // not re-search for (and skip past) a marker that is already
+          // behind the cursor - the exact bug class already fixed once in
+          // IMFBoundary's PREFIX/TAIL split.
+          {
+             MString marker;
+             marker << "--";
+             marker << boundary;
+             int pos = buffer.find(marker);
+             if (pos < 0)
+                return Status::INCOMPLETE;
+             if (!buffer.ifNext(marker))
+                throw coreutils::Exception("CGIFormData: opening boundary vanished between find and ifNext.");
+             state = OPENING_TAIL;
+             return Status::DATA_AVAIL;
+          }
+
+       case OPENING_TAIL:
+          if (buffer.getLength() < 2)
+             return Status::INCOMPLETE;
+          if (buffer.ifNext(ZString("--"))) {
+             state = DONE;
+             return Status::COMPLETE;
+          }
+          if (!buffer.ifNext(ZString("\r\n")))
+             throw coreutils::Exception("CGIFormData: malformed opening boundary line.");
+          state = PART_HEADERS;
+          return Status::DATA_AVAIL;
+
+       case URLENCODED_BODY:
+          if ((int)buffer.getLength() < contentLength)
+             return Status::INCOMPLETE;
+          {
+             ZString body(buffer.getCursor(), contentLength);
+             buffer.setCursor(buffer.getCursor() + contentLength);
+             CGIFormattedData parsed(body);
+             for (auto &pair : parsed.data)
+                storeValue(pair.first, pair.second);
+          }
+          state = DONE;
+          return Status::COMPLETE;
+
+       case PART_HEADERS:
+          if (!headerParser)
+             headerParser = new IMFHeader(reader);
+          if (!checkParse(*headerParser))
+             return Status::INCOMPLETE;
+          extractPartName(*headerParser);
+          delete headerParser;
+          headerParser = NULL;
+          state = PART_BODY;
+          return Status::DATA_AVAIL;
+
+       case PART_BODY: {
+          MString marker;
+          marker << "\r\n--";
+          marker << boundary;
+          char *bodyStart = buffer.getCursor();
+          int pos = buffer.find(marker);
+          if (pos < 0)
+             return Status::INCOMPLETE;
+          char *bodyEnd = buffer.getCursor();
+          if (!currentSkip && currentName.getLength() > 0)
+             storeValue(currentName, MString(ZString(bodyStart, bodyEnd - bodyStart)));
+          currentName = MString();
+          currentSkip = false;
+          state = PART_BOUNDARY;
+          return Status::DATA_AVAIL;
+       }
+
+       case PART_BOUNDARY:
+          if (!boundaryParser)
+             boundaryParser = new IMFBoundary(reader, boundary);
+          if (!checkParse(*boundaryParser))
+             return Status::INCOMPLETE;
+          {
+             bool last = boundaryParser->isLast();
+             delete boundaryParser;
+             boundaryParser = NULL;
+             if (last) {
+                state = DONE;
+                return Status::COMPLETE;
+             }
+             state = PART_HEADERS;
+             return Status::DATA_AVAIL;
+          }
+
+       case DONE:
+          return Status::COMPLETE;
+
+      }
+      throw coreutils::Exception("CGIFormData: unreachable state.");
+   }
+
+   void CGIFormData::storeValue(MString name, MString value) {
+      int index = nameCounts[name]++;
+      MString key = name;
+      key << ":";
+      key << MString(std::to_string(index));
+      data[key] = value;
+   }
+
    MString CGIFormData::operator[](ZString name) {
-      if(formdata == NULL)
-	return MString();
-      return (*formdata).data[name];
+      MString key(name);
+      if (key.find(ZString(":")) == -1) {
+         key << ":0";
+      }
+      return data[key];
    }
 
 }

CGIFormData.h

@@ -1,27 +1,75 @@
 #ifndef __CGIFormData_h__
 # define __CGIFormData_h__
 
-#include "MString.h"
-#include "ZString.h"
-#include "CGIFormattedData.h"
+#include "Parser.h"
+#include "IMFHeader.h"
+#include "IMFBoundary.h"
+#include <map>
 
 namespace coreutils {
 
-   class CGIFormData : public MString {
+   ///
+   /// CGIFormData parses a CGI request body (application/x-www-form-urlencoded
+   /// or multipart/form-data) from the shared StreamReader and exposes the
+   /// resulting name/value pairs via operator[] - the object a caller (e.g.
+   /// JetCore's Tag::getVariable, for its $[:name] CGI variable syntax) holds
+   /// onto and indexes by variable name once doParse() reports COMPLETE.
+   ///
+   /// Resumable like every other Parser here: for multipart/form-data, each
+   /// part's boundary line and header block are parsed via IMFBoundary/
+   /// IMFHeader (chained through checkParse(), sharing this same
+   /// StreamReader), so a part - or even its boundary marker - can straddle
+   /// any number of feed() calls without losing state. File-upload parts
+   /// (Content-Disposition with a filename= parameter) are recognized and
+   /// skipped - matching this codebase's existing convention that uploads
+   /// get a separate mechanism, not buffered through here.
+   ///
+
+   class CGIFormData : public Parser {
 
     public:
-      CGIFormData(int fd, ZString contentType, int contentLength, ZString boundary);
+
+      CGIFormData(StreamReader &reader, ZString contentType, int contentLength, ZString boundary);
+      virtual ~CGIFormData();
 
       MString operator[](ZString name);
 
+    protected:
+
+      Status parse() override;
+
     private:
-      CGIFormattedData *formdata = NULL;
-      MString contentType;
+
+      enum Mode { URLENCODED, MULTIPART, UNKNOWN } mode;
+      enum State { INIT, OPENING_TAIL, PART_HEADERS, PART_BODY, PART_BOUNDARY, URLENCODED_BODY, DONE } state = INIT;
+
+      ZString contentType;
+      int contentLength;
+      ZString boundary;
+      std::map<MString, MString> data;
+
+      IMFHeader *headerParser = NULL;
+      IMFBoundary *boundaryParser = NULL;
+      MString currentName;
+      bool currentSkip = false;
+      std::map<MString, int> nameCounts;
+
+      void extractPartName(IMFHeader &header);
+
+      ///
+      /// Stores value under "name:N", where N is how many values have
+      /// already been stored under this same base name (0 for the first,
+      /// 1 for the second, etc.) - matches the indexed key format
+      /// JetCore's Tag::getVariable's $[:name] syntax looks up by
+      /// (defaulting to :0 when the caller doesn't specify an index),
+      /// supporting repeated field names (checkboxes, multi-select) the
+      /// same way.
+      ///
+
+      void storeValue(MString name, MString value);
 
    };
 
 }
 
 #endif
-
-

IMFHeader.cpp

@@ -40,7 +40,11 @@ namespace coreutils {
 
        case FIELD:
           field << buffer.getTokenExclude("\r\n");
-          if (buffer.eod())
+          // Need 2 bytes buffered to reliably test for "\r\n" - eod() (a
+          // 1-byte check) would wrongly treat a lone trailing \r as
+          // malformed instead of "wait for the \n". Same bug class already
+          // fixed once in IMFBoundary's TAIL state.
+          if (buffer.getLength() < 2)
              return Status::INCOMPLETE;
           if (!buffer.ifNext(ZString("\r\n")))
              throw coreutils::Exception("IMFHeader: expected CRLF at end of header field.");