Fossil Forum

Endless loop in builtin.c:builtin_deliver_multiple_js_files()
Login

Endless loop in builtin.c:builtin_deliver_multiple_js_files()

Endless loop in builtin.c:builtin_deliver_multiple_js_files()

(1) By anonymous on 2022-09-01 15:47:16 [source]

If one will pass a request like "builtin?m=35,&smth" (or something that is not digit) to the fossil binary, builtin_deliver_multiple_js_files() will stuck in the endless loop trying to parse that string.

(2) By Stephan Beal (stephan) on 2022-09-01 16:06:19 in reply to 1 [link] [source]

If one will pass a request like "builtin?m=35,&smth" (or something that is not digit) to the fossil binary, builtin_deliver_multiple_js_files() will stuck in the endless loop trying to parse that string.

This is now fixed in the trunk. Thank you very much for the report!

(3) By anonymous on 2022-09-01 16:19:07 in reply to 2 [link] [source]

It still can get some unrelated numbers, though: "?m=1&n=69" -- here, it will happily skip to the "n" parameter, and will get "69" too. While it is mostly harmless, it may still bundle some unrelated scripts (or bundle some script twice). Maybe also stop parsing on '&'?

(5) By Stephan Beal (stephan) on 2022-09-01 16:30:07 in reply to 3 [link] [source]

It still can get some unrelated numbers, though: "?m=1&n=69"

How are you getting it to read that? The & will get split by the CGI arguments processor long before the builtin-handling code is reached. If you've specifically encoded an & into its argument then it's a clear case of "garbage in, garbage out." The numbers passed to /builtin are strictly fossil-internal (and can change in any given build), as is the list of files hosted by /builtin. Nobody has any business passing any values to it - it's for fossil's own use. Beyond preventing a segfault, security issue, endless loop, or the like, there's no benefit to adding code intended solely to prevent garbage results for garbage inputs.

(4) By anonymous on 2022-09-01 16:26:32 in reply to 2 [link] [source]

Clarification: the thing is that builtin_fulfill_js_requests() sends the request like ?m=...&id=..., where id may start with a correct number that will be parsed by atoi(), so this is not some artificial example, this is what Fossil itself can generate.

(6) By Stephan Beal (stephan) on 2022-09-01 16:31:38 in reply to 4 [link] [source]

the thing is that builtin_fulfill_js_requests() sends the request like ?m=...&id=...,

That function won't get that string, however. Those URL arguments will be parsed by the fossil core in one of the earliest steps before the handler for /builtin is called.

(7) By anonymous on 2022-09-01 16:42:30 in reply to 6 [link] [source]

Yet that's how i discovered the bug in the first place. Actually, the request looked like this: "builtin?m=35%2c15%2c16%2c19%2c17%2c20%2c18%2c30%2c21%2c29%2c31%2c32%26id=52af8d78", and Fossil stuck on the decoded "&". I dumped zList and found "&id" there.

Sure, it may be one of my local patches, but i'm pretty sure that I never ever touched anything related to the URL processing. Sorry, I cannot rebuild and check the clean sources right now...

(8) By Stephan Beal (stephan) on 2022-09-01 16:49:39 in reply to 7 [link] [source]

Actually, the request looked like this:

That looks to be a case of duplicate encoding somewhere. The worst that will happen (since your fix) is that the /builtin request won't serve what was intended. Garbage in, garbage out. If you find that fossil is double-encoding that somewhere, please let us know where because that's a definite bug. My current assumption is that it's coming from a client-written skin or local patches, as this has never been reported before. Since pages which use /builtin won't work properly without their correct /builtin arguments, and we're not aware of any pages which are outright broken, it seems fair to assume that it's generally working how it's supposed to out in the wild.

(9) By anonymous on 2022-09-01 16:59:17 in reply to 8 [link] [source]

That's what I got from my webserver logs. It was among other good requests, so I'm almost 100% sure that it wasn't specifically forged. It may be some browser extension or setting that escapes everything that is not alphanumeric, or something. I cannot replicate it with my own browser, though. Sadly, this is all info I have at my hands (my log doesn't even store a useragent string).