-
-
Notifications
You must be signed in to change notification settings - Fork 88
Comments
Url: optimized unescape() performance#32
Conversation
5692048 to
60209fc
Compare
fprochazka
commented
Dec 18, 2014
Could you also put those cases from the description in the test file?
JanTvrdik
commented
Dec 18, 2014
You mean the ugly long ones?
Another thing to consider: Url::unescape is used by Url::canonicalize however currently Url::canonicalize does not unify %aa and %AA. We may modify Url::unescape to this for us, e.g.
for ($i = 1; $i < count($parts); $i += 2) { $parts[$i] = strtoupper($parts[$i]); }
The performance impact if we choose to do this:
Test split with strtoupper:
a 0 ms
b 0 ms
c 0 ms
d 0 ms
no-escapes (a) 1 ms
no-escapes (z) 1 ms
escapes 9 ms
escapes2 13 ms
escapes3 11 ms
escapes4 156 ms
fprochazka
commented
Dec 18, 2014
Yeah, I think the extreme cases should also be there.
It's still a win :) Much better than before
JanTvrdik
commented
Dec 18, 2014
Btw. I still don't understand the time complexity of those implementations. If I change in the escapes2 dataset the length from 1e5 / 2 to 1e5 (ie. make it twice longer), the new and old tests would became 10 times slower. Maybe I exceed some internal buffer size or I don't know.
It also means that if your web server does not refuse 600KB long URL, you are currently pretty much screwed.
Edit: 1e5 * 2 takes over 120s and a lot over of memory as well (I had to increase the limit from 128M to 512M).
Note that my implementation (split) is still vulnerable to escape4 input. For 2MB URL it takes about 3 second and 400 MB of memory. We should consider setting a hard limit on URL length to avoid this vulnerability because I don't know how to make it any more efficient.
I though about it a bit more and the memory could be optimized by using $limit parameter of preg_split, e.g.
if ($reserved === '') { $s = rawurldecode($s); } else { $pattern = '#((?:%(?:' . implode('|', str_split(bin2hex($reserved), 2)) . '))++)#i'; $res = ''; do { $parts = preg_split($pattern, $s, 1024, PREG_SPLIT_DELIM_CAPTURE); $s = isset($parts[2046]) ? $parts[2046] : ''; unset($parts[2046]); for ($j = 0; $j < count($parts); $j += 2) { $parts[$j] = rawurldecode($parts[$j]); } $res .= implode('', $parts); } while ($s !== ''); $s = $res; }
This makes it for the 2MB worst-case input (escape4) about 2 times slower but the memory consumption is reduces to from 400 MB to 15 MB. Still hard limit seems like the best option. Any tips where to draw the line?
I'll continue talking to myself. I don't think that we should ever exceed 1 second runtime and 10 MB memory usage on this. Therefore the hard limit on URL length should be 256 KiB (which takes 71 ms and 10 MB of memory).
9426286 to
cab8785
Compare
JanTvrdik
commented
Dec 18, 2014
What I love about improving performance is that after you finished one fast implementation – you got an idea how to make it even faster
for ($i = 0; $i < $testCount; $i++) { if ($reserved === '') { $s = rawurldecode($s); } else { $pattern = '#(?:%(?!' . implode('|', str_split(bin2hex($reserved), 2)) . ')[0-9a-f][0-9a-f])++#i'; $s = preg_replace_callback( $pattern, function ($m) { return rawurldecode($m[0]); }, $s ); } }
xificurk
commented
Dec 18, 2014
I don't think it should be Nette's responsibility to limit the URL length. All of the major webservers (apache, nginx, IIS) have by default a limit on URL length, all in the range of couple of kilobytes, therefore your suggested hard limit of 256 KiB is much higher than the defaults. And I assume that if someone changes these defaults then he knows what he's doing.
cab8785 to
f4c06de
Compare
JanTvrdik
commented
Dec 18, 2014
hard limit of 256 KiB is much higher than the defaults
That is intentionally, this limit should be never reached. The question is whether we should rely server configuration or whether Nette should enforce its own limit (large enough to not affect any non-dos requests) as a security fallback.
f4c06de to
8805236
Compare
8805236 to
b05aa50
Compare
fprochazka
commented
Dec 18, 2014
If I'm not mistaken, making it 256KiB means having the limit ~128x larger than what a browser can handle. I have no problem with such a limit.
JanTvrdik
commented
Dec 18, 2014
@fprochazka See http://technomanor.wordpress.com/2012/04/03/maximum-url-size/, every browser except for IE can handle URL with 100 KiB in size.
fprochazka
commented
Dec 18, 2014
@JanTvrdik good to know, In that case, making it 256KiB is still a good limit. I would make it a static property (I can't believe I'm suggesting this) or an argument in that function, so it can be changed if neccesary.
JanTvrdik
commented
Dec 18, 2014
@fprochazka If we choose to not trust the web servers and enforce the limit, it should certainly be in RequestFactory and handle the same way as any other invalid input, i.e. the same way as #30.
Therefore this can be merged regardless of whether we choose to impose a limit on the URL length. cc @dg
fprochazka
commented
Dec 18, 2014
In that case, this optimalization should be solved by it's own and merged. And new issue for the limit should be created. And after the issues we're discussing in #30 are resolved, then we can add the limit.
dg
commented
Dec 18, 2014
Maybe micro-optimization for empty $reserved is unnecessary (is not used by Nette) and can be replaced with another micro-optimization: elimination of "slow" ternary operator in PHP 5.3.
JanTvrdik
commented
Dec 18, 2014
Its not so much a micro-optimization as that the code can not handle empty $reserved. If you see a way how to make the code work with empty $reserved we can remove the condition.
Regarding framework currently not using empty $reserved – I was thinking about changing the second line in canonicalize() to use unescape to improve consistency with the other two lines.
dg
commented
Dec 18, 2014
I didn't realize that it will not work with empty $reserved.
What about unify %aa and %AA?
(It can be maybe done by replacing all reserved chars %aa with %25AA and converting whole result with rawurldecode)
JanTvrdik
commented
Dec 18, 2014
Regarding unification – That could be easily done with the split algorithm but it is much more memory intensive than replace_callback and split-limited is slower and still uses a lot more memory than replace_callback.
The only way I currently see with replace_callback is a second call to preg_replace_callback.
It could be done with a single call but it is still slow:
$xx = implode('|', str_split(bin2hex($reserved), 2)); $actual = preg_replace_callback( '#(?:(?:%(?!' . $xx . ')[0-9a-f]{2})++)|((?:%(?:' . $xx . '))++)#i', function ($m) { return !empty($m[1]) ? strtoupper($m[0]) : rawurldecode($m[0]); }, $s );
dg
commented
Dec 21, 2014
@JanTvrdik this way (#32 (comment))
if ($reserved !== '') { $s = preg_replace_callback( '#%(' . substr(chunk_split(bin2hex($reserved), 2, '|'), 0, -1) . ')#i', function($m) { return '%25' . strtoupper($m[1]); }, $s ); } $s = rawurldecode($s);
JanTvrdik
commented
Dec 21, 2014
Good job! I didn't thought about double escaping.
Currently unescape() can be used to perform DoS on Nette application if HTTP server does not restrict URL length.
Benchmark
Results: