Re: Issues with HTTP multipart/form-data file upload

Xavier Del Campo Romero <[email protected]> Sun, 1 Sep 2024 23:17:53 +0200
Newsgroups	gmane.comp.web.dillo.devel
Message-ID	<[email protected]>
Hi Rodrigo,

> Not sure what you mean with "unnecessary use of the heap". I meant
> something like this: 

You are right: I was too fixated with strcspn(3), when in fact strchr(3)
would already do. Also, this use of dStr_append_c() looks good to me.

> The variable c is stored in a register, as you can see in the
> disassembly (built with -Og): 

Thanks for the detailed explanation. Anyway, I do not expect this
function to become a performance bottleneck.

> This other method has a slightly not
> uniform distribution, but only runs rand() 70 times: 
> 
> Which is probably okay for this case.

I also think it is good enough.

> Then, I think using our own set may be the most readable solution here,
> which also allows the quick module method. 

I agree.

Best regards,

Xavi

On 1/9/24 16:12, Rodrigo Arias wrote:
> Hi Xavier,
> 
>> Thank you. I am still unfamiliar with that part of Dillo, so please let
>> me know about any progress.
> 
> For now I'm still writing the RFC and doing some proof of concepts. I
> can see that it will take a while.
> 
>> Limiting ourselves to a-z, A-Z and 0-9 would still account for 62 out of
>> the 75 possible characters, so roughly 82% of the set. I think that
>> removing the quoting in favour of the limited set reduce the risk for
>> broken implementations, yet still provide a good amount of randomness.
> 
> Yes, I think so too.
> 
>> I am not sure whether this was an intentional modification from your
>> side. My patch is adding a <space> as defined by POSIX.1-2017 [1], so
>> that sizeof " " would always return 2. Was it your intention to flag
>> this potential confusion?
> 
> Yes, my point was that it is that is not easy to determine the length of
> a UTF-8 string by just looking at it.
> 
>> Also, there was not strict reason to use sizeof " ". Any other
>> character would do e.g.: sizeof "x", sizeof "A", etc.
> 
> Same problem:
> 
> ᕁ 𝓍 𝙭 х 𝐱 𝗑 ⤫ 𝑥 𝘅 ⤬ ᙮ ⨯ 𝕩 𝖝 × 𝔁 𝚡 x ｘ ⅹ 𝔵 ᕽ 𝒙 𝘹
> 
> 𝙰 A 𝐴 ᗅ 𝑨 𝚨 𝕬 𝖠 А Α 𝛢 𝝖 𝒜 𝜜 𖽀 𝓐 𝞐 Ａ 𝗔 𝘈 Ꭺ ꓮ 𝐀 𝔄 𝔸 𐊠 𝘼
> 
> Check: https://util.unicode.org/UnicodeJsps/confusables.jsp
> 
> It is generally safer to use the explicit length.
> 
>>> You can also use dStr_append_c() to only append one character, so you
>>> only need a single character.
>>
>> That would be an unnecessary use of the heap, because the size is static.
> 
> Not sure what you mean with "unnecessary use of the heap". I meant
> something like this:
> 
>   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";
>         int c;
> 
>         do {
>            c = rand() & 0xff;
>         } while (!strchr(set, c));
> 
>         dStr_append_c(boundary, c);
>      }
>   }
> 
> The variable c is stored in a register, as you can see in the
> disassembly (built with -Og):
> 
> hop% r2 build/src/dillo
> WARN: Relocs has not been applied. Please use `-e bin.relocs.apply=true`
> or `-e bin.cache=true` next time
> [0x000e3860]> s sym.generate_boundary_Dstr_
> [0x0012cf99]> af
> [0x0012cf99]> pdf
> ┌ 85: sym.generate_boundary_Dstr_ (int64_t arg1);
> │           ; arg int64_t arg1 @ rdi
> │           0x0012cf99      55             push rbp                    ;
> Fl_Pixmap.H:1250 ; generate_boundary(Dstr*)
> │           0x0012cf9a      4889e5         mov rbp, rsp
> │           0x0012cf9d      4155           push r13
> │           0x0012cf9f      4154           push r12
> │           0x0012cfa1      53             push rbx
> │           0x0012cfa2      4883ec08       sub rsp, 8
> │           0x0012cfa6      4989fd         mov r13, rdi                ;
> arg1
> │           0x0012cfa9      41bc00000000   mov r12d, 0                 ;
> Fl_Pixmap.H:1251
> │       ┌─< 0x0012cfaf      eb2c           jmp 0x12cfdd
> │     ┌┌──> 0x0012cfb1      ff1509cd1e00   call qword [reloc.rand]     ;
> Fl_Pixmap.H:1253 ; [0x319cc0:8]=0
> │     ╎╎│   0x0012cfb7      0fb6d8         movzx ebx, al               ;
> <--- Perform the "& 0xff" by zero-extending the lowest byte in eax
> │     ╎╎│   0x0012cfba      89de           mov esi, ebx                ;
> Fl_Pixmap.H:1260
> │     ╎╎│   0x0012cfbc      488d3dbd5b..   lea rdi,
> obj.generate_boundary_Dstr_::set ; 0x2a2b80 ;
> "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
> │     ╎╎│   0x0012cfc3      ff15afc51e00   call qword [reloc.strchr]   ;
> [0x319578:8]=0
> │     ╎╎│   0x0012cfc9      4885c0         test rax, rax
> │     └───< 0x0012cfcc      74e3           je 0x12cfb1
> │      ╎│   0x0012cfce      89de           mov esi, ebx                ;
> Fl_Pixmap.H:1262
> │      ╎│   0x0012cfd0      4c89ef         mov rdi, r13
> │      ╎│   0x0012cfd3      67e830570800   call sym.dStr_append_c
> │      ╎│   0x0012cfd9      4183c401       add r12d, 1                 ;
> Fl_Pixmap.H:1251
> │      ╎│   ; CODE XREF from generate_boundary(Dstr*) @ 0x12cfaf(x)
> │      ╎└─> 0x0012cfdd      4183fc45       cmp r12d, 0x45              ;
> 'E'
> │      └──< 0x0012cfe1      7ece           jle 0x12cfb1
> │           0x0012cfe3      4883c408       add rsp, 8                  ;
> Fl_Pixmap.H:1264
> │           0x0012cfe7      5b             pop rbx
> │           0x0012cfe8      415c           pop r12
> │           0x0012cfea      415d           pop r13
> │           0x0012cfec      5d             pop rbp
> └           0x0012cfed      c3             ret
> 
> This method gives us a perfect uniform distribution when RAND_MAX is a
> power of 2, at the cost of executing rand() more times than required
> (70/(62/256) ≈ 289 on average). This other method has a slightly not
> uniform distribution, but only runs rand() 70 times:
> 
>   static void generate_boundary(Dstr *boundary)
>   {
>      /* Extracted from RFC 2046, section 5.1.1. */
>      static const char set[] = "abcdefghijklmnopqrstuvwxyz"
>         "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
>         "0123456789";
>      static const int n = strlen(set);
> 
>      for (int i = 0; i < 70; i++) {
>         int c = (unsigned char) set[rand() % n];
>         dStr_append_c(boundary, c);
>      }
>   }
> 
> Which is probably okay for this case.
> 
> Notice I haven't tested any of these methods yet, I'll need to add a
> form upload test case to be able to see them in action (or a unit test).
> 
>>> If we only use alphanumeric characters, we can just use isalnum() right?
>>
>> According to POSIX.1-2017 [2], isalnum(3) depends on the current locale
>> configured by the system. For example, characters such as Ä or ú could
>> return non-zero.
> 
> Oh right, for some reason I was thinking this was the other way around,
> and isalnum only worked with ASCII. I think we should review the other
> uses of isalnum and friends as I think there may be used under similar
> assumptions.
> 
>> To avoid this, there are two possible solutions:
>>
>> 1. Use isalnum_l(3) to specify a locale_t object corresponding to the
>> "POSIX" locale (equivalent to "C" [3]), which must be previously
>> allocated by the newlocale(3) function [3] and released by the
>> freelocal(3) function [4]. A minimalist example is shown below:
>>
>>        locale_t l = newlocale(LC_CTYPE, "POSIX", NULL);
>>
>>        for (unsigned char i = 0; i < 255; i++)
>>                printf("hhu=%hhu, c=%c, isalnum=%d\n", i, i,
>> isalnum_l(i, l));
>>
>>        freelocale(l);
>>
>> 2. Define a known subset from the portable character set defined by
>> POSIX.1-2017 [5] and use strspn(3), as already suggested by the patch.
>> IMHO this approach is better because:
>>     - It does not deal with locales, so developers not familiar with them
>> would understand the code better.
>>     - It is also portable outside a POSIX environment (not sure if this a
>> requirement, though).
>>     - It does not require dynamic allication via newlocale(3).
>>     - It is the only possible option if non-alnum characters, such as ':'
>> or '/', are appended to the boundary string.
> 
> Then, I think using our own set may be the most readable solution here,
> which also allows the quick module method.
> 
> Best,
> Rodrigo
> _______________________________________________
> Dillo-dev mailing list -- [email protected]
> To unsubscribe send an email to [email protected]

_______________________________________________
Dillo-dev mailing list -- dillo-dev-lx9mn2B4QYRWk0Htik3J/[email protected]
To unsubscribe send an email to dillo-dev-leave-lx9mn2B4QYRWk0Htik3J/[email protected]
OpenPGP_0x84FF3612A9BF43F2.asc (application/pgp-keys, 4.8 KB)
OpenPGP_signature.asc (application/pgp-signature, 840 B)