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