Issues with HTTP multipart/form-data file upload

Xavier Del Campo Romero <[email protected]> Mon, 26 Aug 2024 01:27:29 +0200

Newsgroups	gmane.comp.web.dillo.devel
Message-ID	<[email protected]>

Hello Dillo dev community,

I am testing support among web browsers for slcl [1], a JS-less
minimalist web storage solution. slcl relies on
"multipart/form-data"-encoded HTTP requests to upload files to a server.
Whereas Dillo helped me to uncover a few wrong assumptions on my code, I
have realised a few issues on Dillo itself related to filue uploads that
should be considered.

Issue #1:

While Dillo is fine uploading small files (up to a few MiB), things go
wrong with larger files, so much that memory usage increases up to
multiple GiB and can even lock the system up. This is because Dillo is
designed to send requests always from memory, which means file contents
must be dumped into memory first, and this might be unfeasible for large
files.

Suggestion:

Instead, Dillo should ideally send file contents on-the-fly, so that
memory usage is kept to a minimum regardless the file size.

Issue #2:

Even if Dillo generates a 70-byte, random boundary string (yet mostly
filled with '-', similarly to Firefox [2]), it ensures it is not found
anywhere inside the file contents. Again, this can be a serious
bottleneck in the case of large files, as it requires to scan the whole
file for a match.

Suggestion:

Define *all* of the 70 bytes in the boundary string as random, and
assume they would never be found inside a file. The chance of accidental
collision is so low that it is not worth the effort into checking them.

Suggested patches:

- 0001-dialog.cc-Generate-more-random-boundaries.patch)

Issue #3:

Dillo only supports uploading 1 file at a time. This is mostly because
it relies (probably temporarily?) on the a_Dialog_save_file function
[3]. However, this is not a limitation on the HTTP protocol, and other
implementation such as Firefox or Chromium-based browsers support this.

Suggested patches:

- 0002-dialog-Add-a_Dialog_select_files.patch
- 0003-WIP-multi-file-uploads.patch

Conclusions:

Dillo seems designed to always send requests from memory, so it is not
straightforward to break this assumption in order to support large file
uploads. 0003-WIP-multi-file-uploads.patch is an incomplete first step
into fixing this, but it surely needs deeper design changes.

I did not put more effort into these patches for the time being because,
after seeing the potential complexity behind this task, I thought it was
a better idea to ask the community for feedback and guidelines.

Thank you very much for reading.

Best regards,

Xavier Del Campo Romero

[1]: https://gitea.privatedns.org/xavi/slcl
[2]:
https://github.com/dillo-browser/dillo/blob/8a360e32ac3136494a494379a6dbbacef6f95da2/src/form.cc#L1299-L1301
[3]:
https://github.com/dillo-browser/dillo/blob/8a360e32ac3136494a494379a6dbbacef6f95da2/src/dialog.cc#L256-L264

_______________________________________________
Dillo-dev mailing list -- dillo-dev-lx9mn2B4QYRWk0Htik3J/[email protected]
To unsubscribe send an email to dillo-dev-leave-lx9mn2B4QYRWk0Htik3J/[email protected]

0001-dialog.cc-Generate-more-random-boundaries.patch (text/x-patch, 2.6 KB)

From 432b8127904abed1dfb178d46cfcd39aaddbc259 Mon Sep 17 00:00:00 2001
From: Xavier Del Campo Romero <[email protected]>
Date: Mon, 26 Aug 2024 01:10:16 +0200
Subject: [PATCH 1/3] dialog.cc: Generate more random boundaries

Even if major implementations tend to add several '-' characters to
multipart/form-data boundaries, this is not enforced by RFC 2046.

Making this boundary string more random would allow Dillo to assume the
boundary string would never be found inside the file to upload.
---
 src/dialog.cc |  1 +
 src/form.cc   | 30 ++++++++++++++++++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/dialog.cc b/src/dialog.cc
index ac007315..0137472b 100644
--- a/src/dialog.cc
+++ b/src/dialog.cc
@@ -14,6 +14,7 @@
  */
 
 #include <math.h> // for rint()
+#include <string.h>
 
 #include <FL/fl_ask.H>
 #include <FL/Fl_Window.H>
diff --git a/src/form.cc b/src/form.cc
index 93bd4864..6e81edf6 100644
--- a/src/form.cc
+++ b/src/form.cc
@@ -1246,6 +1246,24 @@ Dstr *DilloHtmlForm::buildQueryData(DilloHtmlInput *active_submit)
    return DataStr;
 }
 
+static void generate_boundary(Dstr *boundary)
+{
+   for (int i = 0; i < 70; i++) {
+      /* Extracted from RFC 2046, section 5.1.1. */
+      static const char set[] = "abcdefghijklmnopqrstuvwxyz"
+         "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+         "0123456789"
+         "'()+_,-./:=? ";
+      char s[sizeof " "] = {0};
+
+      do {
+         *s = rand();
+      } while (!strspn(s, set));
+
+      dStr_append(boundary, s);
+   }
+}
+
 /**
  * Generate a boundary string for use in separating the parts of a
  * multipart/form-data submission.
@@ -1253,7 +1271,6 @@ Dstr *DilloHtmlForm::buildQueryData(DilloHtmlInput *active_submit)
 char *DilloHtmlForm::makeMultipartBoundary(iconv_t char_encoder,
                                            DilloHtmlInput *active_submit)
 {
-   const int max_tries = 10;
    Dlist *values = dList_new(5);
    Dstr *DataStr = dStr_new("");
    Dstr *boundary = dStr_new("");
@@ -1294,15 +1311,8 @@ char *DilloHtmlForm::makeMultipartBoundary(iconv_t char_encoder,
       }
    }
 
-   /* generate a boundary that is not contained within the data */
-   for (int i = 0; i < max_tries && !ret; i++) {
-      // Firefox-style boundary
-      dStr_sprintf(boundary, "---------------------------%d%d%d",
-                   rand(), rand(), rand());
-      dStr_truncate(boundary, 70);
-      if (dStr_memmem(DataStr, boundary) == NULL)
-         ret = boundary->str;
-   }
+   generate_boundary(boundary);
+   ret = boundary->str;
    dList_free(values);
    dStr_free(DataStr, 1);
    dStr_free(boundary, (ret == NULL));
-- 
2.34.1

0002-dialog-Add-a_Dialog_select_files.patch (text/x-patch, 2.9 KB)

From de375a3e0dbe2c4f2d341cf2804ef4e7ef9862c4 Mon Sep 17 00:00:00 2001
From: Xavier Del Campo Romero <[email protected]>
Date: Mon, 26 Aug 2024 01:16:43 +0200
Subject: [PATCH 2/3] dialog: Add a_Dialog_select_files

As its name suggests, and as opposed to a_Dialog_select_file, this
function allows to select multiple files from a directory.

This commit is meant to allow multi-file uploads by future commits.
---
 src/dialog.cc | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/dialog.hh |  4 ++++
 2 files changed, 69 insertions(+)

diff --git a/src/dialog.cc b/src/dialog.cc
index 0137472b..6e4f6e6d 100644
--- a/src/dialog.cc
+++ b/src/dialog.cc
@@ -278,6 +278,71 @@ char *a_Dialog_open_file(const char *title,
    return (fc_name) ? a_Misc_escape_chars(fc_name, "% #") : NULL;
 }
 
+/* Implementation based on upstream fltk: src/fl_file_dir.cxx */
+static void show_file_chooser(Fl_File_Chooser *chooser)
+{
+   Fl_Window *g;
+
+   chooser->show();
+   g = Fl::grab();
+
+   if (g)
+      Fl::grab(0);
+
+   while (chooser->shown())
+      Fl::wait();
+
+   if (g)
+      Fl::grab(g);
+}
+
+Dlist *a_Dialog_select_files(const char *title,
+                        const char *pattern, const char *fname)
+{
+   Dlist *list = NULL;
+   Fl_File_Chooser chooser(fname, pattern, Fl_File_Chooser::MULTI, title);
+   int n;
+
+   show_file_chooser(&chooser);
+   n = chooser.count();
+
+   if (n <= 0) {
+      goto failure;
+   }
+
+   list = dList_new(n);
+
+   if (!list) {
+      goto failure;
+   }
+
+   for (int i = 0; i < n; i++) {
+      const char *fc_name = chooser.value(i);
+      char *escaped;
+
+      if (!fc_name) {
+         goto failure;
+      } else if (!(escaped = a_Misc_escape_chars(fc_name, "% #"))) {
+         goto failure;
+      }
+
+      /* dList_apend has no way to tell whether the operation was successful.*/
+      dList_append(list, escaped);
+   }
+
+   return list;
+
+failure:
+
+   if (list) {
+      for (int i = 0; list->len; i++)
+         free(list->list[i]);
+   }
+
+   dList_free(list);
+   return NULL;
+}
+
 /**
  * Close text window.
  */
diff --git a/src/dialog.hh b/src/dialog.hh
index 0a489590..4a360494 100644
--- a/src/dialog.hh
+++ b/src/dialog.hh
@@ -1,6 +1,8 @@
 #ifndef __DIALOG_HH__
 #define __DIALOG_HH__
 
+#include "dlib/dlib.h"
+
 #ifdef __cplusplus
 extern "C" {
 #endif /* __cplusplus */
@@ -18,6 +20,8 @@ const char *a_Dialog_save_file(const char *title,
                                const char *pattern, const char *fname);
 const char *a_Dialog_select_file(const char *title,
                                  const char *pattern, const char *fname);
+Dlist *a_Dialog_select_files(const char *title,
+                         const char *pattern, const char *fname);
 char *a_Dialog_open_file(const char *title,
                          const char *pattern, const char *fname);
 void a_Dialog_text_window(const char *title, const char *txt);
-- 
2.34.1

0003-WIP-multi-file-uploads.patch (text/x-patch, 8.5 KB)

From 915e8c34973258955ab3170ca43fca00b4207e4e Mon Sep 17 00:00:00 2001
From: Xavier Del Campo Romero <[email protected]>
Date: Mon, 26 Aug 2024 01:18:42 +0200
Subject: [PATCH 3/3] WIP multi-file uploads

TODO: send file data on-the-fly instead of dumping file contents into
memory.
---
 src/dialog.cc | 15 -----------
 src/dialog.hh |  2 --
 src/form.cc   | 75 +++++++++++++++++++++++++++++++++++++--------------
 src/uicmd.cc  |  4 +--
 src/uicmd.hh  |  3 ++-
 5 files changed, 59 insertions(+), 40 deletions(-)

diff --git a/src/dialog.cc b/src/dialog.cc
index 6e4f6e6d..e818d028 100644
--- a/src/dialog.cc
+++ b/src/dialog.cc
@@ -249,21 +249,6 @@ const char *a_Dialog_save_file(const char *title,
    return fl_file_chooser(title, pattern, fname);
 }
 
-/**
- * Show the select file dialog.
- *
- * @return pointer to chosen filename, or NULL on Cancel.
- */
-const char *a_Dialog_select_file(const char *title,
-                                 const char *pattern, const char *fname)
-{
-   /*
-    * FileChooser::type(MULTI) appears to allow multiple files to be selected,
-    * but just follow save_file's path for now.
-    */
-   return a_Dialog_save_file(title, pattern, fname);
-}
-
 /**
  * Show the open file dialog.
  *
diff --git a/src/dialog.hh b/src/dialog.hh
index 4a360494..61a65090 100644
--- a/src/dialog.hh
+++ b/src/dialog.hh
@@ -18,8 +18,6 @@ const char *a_Dialog_input(const char *title, const char *msg);
 const char *a_Dialog_passwd(const char *title, const char *msg);
 const char *a_Dialog_save_file(const char *title,
                                const char *pattern, const char *fname);
-const char *a_Dialog_select_file(const char *title,
-                                 const char *pattern, const char *fname);
 Dlist *a_Dialog_select_files(const char *title,
                          const char *pattern, const char *fname);
 char *a_Dialog_open_file(const char *title,
diff --git a/src/form.cc b/src/form.cc
index 6e81edf6..b1407ab7 100644
--- a/src/form.cc
+++ b/src/form.cc
@@ -13,6 +13,7 @@
 #include "html_common.hh"
 
 #include <errno.h>
+#include <stdio.h>
 #include <iconv.h>
 
 #include "lout/misc.hh"
@@ -150,13 +151,12 @@ public:  //BUG: for now everything is public
                          entries, it is the initial value */
    DilloHtmlSelect *select;
    bool init_val;     /* only meaningful for buttons */
-   Dstr *file_data;   /* only meaningful for file inputs.
-                         TODO: may become a list... */
+   Dlist *file_list;   /* only meaningful for file inputs. */
 
 private:
    void connectTo(DilloHtmlReceiver *form_receiver);
    void activate(DilloHtmlForm *form, int num_entry_fields,EventButton *event);
-   void readFile(BrowserWindow *bw);
+   void getFileList(BrowserWindow *bw);
 
 public:
    DilloHtmlInput (DilloHtmlInputType type, Embed *embed,
@@ -1175,6 +1175,7 @@ Dstr *DilloHtmlForm::buildQueryData(DilloHtmlInput *active_submit)
 
          if ((valcount = dList_length(values)) > 0) {
             if (input->type == DILLO_HTML_INPUT_FILE) {
+#if 0
                if (valcount > 1)
                   MSG_WARN("multiple files per form control not supported\n");
                Dstr *file = (Dstr *) dList_nth_data(values, 0);
@@ -1195,6 +1196,7 @@ Dstr *DilloHtmlForm::buildQueryData(DilloHtmlInput *active_submit)
                   dStr_free(dfilename, 1);
                }
                dStr_free(file, 1);
+#endif
             } else if (input->type == DILLO_HTML_INPUT_INDEX) {
                /* no name */
                Dstr *val = (Dstr *) dList_nth_data(values, 0);
@@ -1290,6 +1292,7 @@ char *DilloHtmlForm::makeMultipartBoundary(iconv_t char_encoder,
          dStr_free(dstr, 1);
       }
       if (input->type == DILLO_HTML_INPUT_FILE) {
+#if 0
          LabelButtonResource *lbr =
             (LabelButtonResource*)input->embed->getResource();
          const char *filename = lbr->getLabel();
@@ -1299,6 +1302,7 @@ char *DilloHtmlForm::makeMultipartBoundary(iconv_t char_encoder,
             dStr_append_l(DataStr, dstr->str, dstr->len);
             dStr_free(dstr, 1);
          }
+#endif
       }
       int length = dList_length(values);
       for (int i = 0; i < length; i++) {
@@ -1659,7 +1663,8 @@ void DilloHtmlReceiver::clicked (Resource *resource,
  */
 DilloHtmlInput::DilloHtmlInput (DilloHtmlInputType type2, Embed *embed2,
                                 const char *name2, const char *init_str2,
-                                bool init_val2)
+                                bool init_val2) :
+   file_list(NULL)
 {
    type = type2;
    embed = embed2;
@@ -1675,7 +1680,6 @@ DilloHtmlInput::DilloHtmlInput (DilloHtmlInputType type2, Embed *embed2,
    default:
       break;
    }
-   file_data = NULL;
    reset ();
 }
 
@@ -1686,7 +1690,14 @@ DilloHtmlInput::~DilloHtmlInput ()
 {
    dFree(name);
    dFree(init_str);
-   dStr_free(file_data, 1);
+
+   if (file_list) {
+      for (int i = 0; i < file_list->len; i++)
+         free(file_list->list[i]);
+   }
+
+   dList_free(file_list);
+
    if (select)
       delete select;
 }
@@ -1721,7 +1732,7 @@ void DilloHtmlInput::activate(DilloHtmlForm *form, int num_entry_fields,
 {
    switch (type) {
    case DILLO_HTML_INPUT_FILE:
-      readFile (form->html->bw);
+      getFileList(form->html->bw);
       break;
    case DILLO_HTML_INPUT_RESET:
    case DILLO_HTML_INPUT_BUTTON_RESET:
@@ -1747,21 +1758,32 @@ void DilloHtmlInput::activate(DilloHtmlForm *form, int num_entry_fields,
 /**
  * Read a file into cache
  */
-void DilloHtmlInput::readFile (BrowserWindow *bw)
-{
-   const char *filename = a_UIcmd_select_file();
-   if (filename) {
-      a_UIcmd_set_msg(bw, "Loading file...");
-      dStr_free(file_data, 1);
-      file_data = a_Misc_file2dstr(filename);
-      if (file_data) {
-         a_UIcmd_set_msg(bw, "File loaded.");
-         LabelButtonResource *lbr = (LabelButtonResource*)embed->getResource();
-         lbr->setLabel(filename);
-      } else {
-         a_UIcmd_set_msg(bw, "ERROR: can't load: %s", filename);
+void DilloHtmlInput::getFileList (BrowserWindow *bw)
+{
+   file_list = a_UIcmd_select_files();
+
+   if (!file_list)
+      return;
+
+   LabelButtonResource *lbr = (LabelButtonResource*)embed->getResource();
+   char buf[sizeof "18446744073709551615 files selected"];
+   const char *label;
+
+   if (file_list->len > 1) {
+      int n = snprintf(buf, sizeof(buf), "%d files selected",
+         file_list->len);
+
+      if (n < 0 || (size_t)n >= sizeof(buf)) {
+         printf("getFileList: snprintf(3) returned %d", n);
+         return;
       }
+
+      label = buf;
+   } else {
+      label = (const char *)file_list->list[0];
    }
+
+   lbr->setLabel(label);
 }
 
 /**
@@ -1810,6 +1832,16 @@ void DilloHtmlInput::appendValuesTo(Dlist *values, bool is_active_submit)
       break;
    case DILLO_HTML_INPUT_FILE:
       {
+         if (!file_list) {
+            fprintf(stderr, "Upload file list is empty\n");
+            break;
+         }
+
+         for (int i = 0; i < file_list->len; i++) {
+            dList_append(values, dStr_new((const char *)file_list->list[i]));
+         }
+
+#if 0
          LabelButtonResource *lbr = (LabelButtonResource*)embed->getResource();
          const char *filename = lbr->getLabel();
          if (filename[0] && strcmp(filename, init_str)) {
@@ -1821,6 +1853,9 @@ void DilloHtmlInput::appendValuesTo(Dlist *values, bool is_active_submit)
                MSG("FORM file input \"%s\" not loaded.\n", filename);
             }
          }
+#else
+#warning TODO
+#endif
       }
       break;
    case DILLO_HTML_INPUT_IMAGE:
diff --git a/src/uicmd.cc b/src/uicmd.cc
index 187aeabc..c66e2c30 100644
--- a/src/uicmd.cc
+++ b/src/uicmd.cc
@@ -1070,9 +1070,9 @@ void a_UIcmd_save(void *vbw)
 /*
  * Select a file
  */
-const char *a_UIcmd_select_file()
+Dlist *a_UIcmd_select_files()
 {
-   return a_Dialog_select_file("Dillo: Select a File", NULL, NULL);
+   return a_Dialog_select_files("Dillo: Select a File", NULL, NULL);
 }
 
 /*
diff --git a/src/uicmd.hh b/src/uicmd.hh
index 0a5c8fb5..deeec29e 100644
--- a/src/uicmd.hh
+++ b/src/uicmd.hh
@@ -14,6 +14,7 @@
 #define __UICMD_HH__
 
 #include "bw.h"
+#include "dlib/dlib.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -44,7 +45,7 @@ void a_UIcmd_stop(void *vbw);
 void a_UIcmd_tools(void *vbw, int x, int y);
 void a_UIcmd_save_link(BrowserWindow *bw, const DilloUrl *url);
 void a_UIcmd_open_file(void *vbw);
-const char *a_UIcmd_select_file(void);
+Dlist *a_UIcmd_select_files(void);
 void a_UIcmd_search_dialog(void *vbw);
 const char *a_UIcmd_get_passwd(const char *user);
 void a_UIcmd_book(void *vbw);
-- 
2.34.1

OpenPGP_0x84FF3612A9BF43F2.asc (application/pgp-keys, 4.8 KB)

OpenPGP_signature.asc (application/pgp-signature, 840 B)