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 -- dillo-dev@mailman3.com To unsubscribe send an email to dillo-dev-leave@mailman3.com