Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the message size restriction and fix the validation of images #2472

Merged
merged 1 commit into from Aug 28, 2016

Conversation

@SuperNascher
Copy link
Contributor

commented Jul 26, 2016

A rework to #2049 and it fixes the validation of image URLs.

@SuperNascher SuperNascher changed the title Changes the behavior for oversized messages and fix the validation of images Remove the restriction of the message size and fix the validation of images Jul 26, 2016

@SuperNascher SuperNascher force-pushed the SuperNascher:message_size branch from c20e7a6 to 380ee61 Jul 26, 2016

@SuperNascher SuperNascher changed the title Remove the restriction of the message size and fix the validation of images Remove the message size restriction and fix the validation of images Jul 26, 2016

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

Code-wise, PR looks fine.

But we need to tread carefully to ensure we don't do something stupid. We need to test how Qt reacts with big HTML documents in the log view. That's the thing I worry about.

Alternatively, we could perhaps limit the documents by area instead of the arbitrary restrictions we currently use?

I'm thinking: the inserted log item must not exceed an area of 4096x4096 (16777216), or 2048x2048 (4194304) -- and perhaps those values are too optimistic. This would a square 2048x2048 log entry. But also a log entry that's sized at 300x11288, or at 11288x300 -- both areas are smaller than 4194304.

@SuperNascher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2016

Some screenshots with and without the restriction:

Without restriction

https://files.nascher.org/mumble_development/mumble_log_without_size_restriction_0.png

https://files.nascher.org/mumble_development/mumble_log_without_size_restriction_1.png

https://files.nascher.org/mumble_development/mumble_log_without_size_restriction_2.png

With restriction

https://files.nascher.org/mumble_development/mumble_log_with_size_restriction_0.png

https://files.nascher.org/mumble_development/mumble_log_with_size_restriction_1.png

https://files.nascher.org/mumble_development/mumble_log_with_size_restriction_2.png

The HTML text from the screenshots:
https://n0paste.tk/0W2D6Qf/

@SuperNascher SuperNascher force-pushed the SuperNascher:message_size branch from 1ffdc2d to 6437183 Jul 28, 2016

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

@SuperNascher I am more concerned about memory usage with large HTML messages, and log performance in general with big messages.

Have you tested any of that? :)

Also I'm interested HTML that displays something huge, but is actually very small in source form, i.e.

<div style="width: 280px; height: 11100px; background-color: blue;"></div>
return errorMessage;
}
int messageSize = s.width() * s.height();
int allowedSize = 300 * 11288;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 28, 2016

Member

Should include a comment about this being a "max area", so as long as the bounding box of the rendered HTML stays within this area, it will be allowed.

Also: why 300 * 11288 instead of something like 2048x2048?

@SuperNascher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2016

@mkrautz
There was no reason for the magic numbers ;-)
I will correct it in some minutes.

Qt supports width and height only on tables, images, word-spacing and horizontal lines:
http://doc.qt.io/qt-5/richtext-html-subset.html#block-attributes

I have tested it out and the width and height styles does not change the style of the text.

Only if I use images with width and height, I can bypass the limit.
<img src="http://m.memegen.com/3p8252.jpg" width="100000" height="100000"/>
Maybe a restriction for that?

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 30, 2016

@SuperNascher can you elaborate on what the strikethrough in the message above means? :)

@SuperNascher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2016

My first thought was, that it is not possible to use width and height to create large HTML elements. Qt allows the use of these styles only for pictures, tables and “horizontal rulers”. Another possibility to abuse the text messages is to use margin-* (margin-top, -bottom, etc.) and padding-* to “extend” the messages. Here are some examples:

Guy On Phone: “Is this the Krusty Krab?”
<p style="margin-top: 300px; margin-left: 20px; background-color: blue; color: white">Patrick: “No, this is Patrick.”</p>

or

<img src="https://wiki.mumble.info/logo.png" height="50000000000" width="50000000000" />

And now comes the question, how Mumble should treat these messages. An approach to the solution could be to create a regexp filter that deletes the margin and padding styles before the validation of the message begins. Another possibility is that Mumble still checks if the message is smaller than the width of a half screen size. If the message is bigger than the message will be posted as HTML source code.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 31, 2016

@SuperNascher So you're saying that if a message includes, say, an image with width=50000 height=50000, then it will not be filtered by the check in this PR?

I'm just trying to understand what the problem is.

@SuperNascher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2016

Yes, the image will not be filtered by this check. The big image even works on the normal mumble version. I have created an issue #2477.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

@SuperNascher Now that we've fixed the big image problem -- does this fix now catch them?
Just trying to move forward with the PR :-)

@SuperNascher SuperNascher force-pushed the SuperNascher:message_size branch 2 times, most recently from 1ddbe6b to e72a029 Aug 6, 2016

} else {
return errorMessage;
}
if (!valid) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 7, 2016

Member

I would move this above qtd.adjustSize(), etc. -- valid is already set there, so we don't need to wait to use it until we're down here.

int messageSize = s.width() * s.height();
int allowedSize = 2048 * 2048;

if (messageSize > allowedSize || !s.isValid()) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 7, 2016

Member

I would prefer it if (!s.isValid()) was its own section like this.

Something like:

if (!is.isValid()) {
    QString errorInvalidSizeMessage = tr("[[ Invalid size ]]");
    if (tc) {
       tc->insertText(errorInvalidSizeMessage);
       return QString();
    } else {
       return errorInvalidSizeMessage;
   }
}

and I'd do the check just after we've retrieved QSizeF s.

Then, the section below this would just be...

int messageSize = s.width() * s.height();
int allowedSize = 2048 * 2048;

if (messageSize > allowedSize) {
   [...]
}
@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 7, 2016

Also still interested in some performance/memory usage.

I think it should be possible to get Qt to render somthing that isn't an image, that is still large?...

Log: introduce new 2048x2048 message area size restriction.
Currently, Mumble has a restrictive message filter
that will filter away messages that are larger
than the screen. A filtered message is replaced with
the text "Text object too large to display".

This commit replaces the existing size filter.
Instead of using the screen size to determine
whether a message is too large to show, Mumble
now checks whether the area of the received
message exceeds 2048x2048.

For example, this will allow messages with
sizes such as 500x8388 or 1500x2796 -- or
simply 2048x2048. As long as the total area
of the rendered message does not exceed
2048x2048 -- in which case the
"Text object too large to display" will
be displayed.

Fixes #2467
@SuperNascher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2016

I am done with the tests. I used QElapsedTimer to track the executing time of validHtml and ps to track the process and memory usage.

Here is the source code for testing (works only on Unix systems): https://n0paste.tk/wNS2zJX/

I have tested three messages and these messages has been tested three times.

First message: https://n0paste.tk/gLH94fJ/
Second message: https://n0paste.tk/l1Sa9JZ/
Third message: https://n0paste.tk/bfkvF4q/

Used terms:
VSZ = Virtual Memory Size in KiB (Kibibyte)
RSS = Resident Set Size, the non-swapped physical memory that a task has used in KB (Kilobytes)

PR Client: The Client with this Pull Request
Original Client: The dev client with this commit: e7ff17b

First message

Test Mumble Client Execution time PID %CPU %MEM VSZ RSS
First -- -- -- --- --- - -
-- Orginal Client 1 ms 8611 7.4 1.4 1597700 120568
-- PR Client 2 ms 7865 9.1 1.4 1597672 117756
Second -- -- -- --- --- - -
-- Orginal Client 1 ms 8747 6.6 1.4 1596664 115408
-- PR Client 1 ms 9082 8.0 1.4 1597672 117000
Third -- -- -- --- --- - -
-- Orginal Client 1 ms 9626 7.8 1.4 1597696 116904
-- PR Client 1 ms 9288 7.1 1.5 1671416 121176
Summary -- -- -- --- --- - -
-- Orginal Client 1 ms -- 7.27 1.4 1597353,33 117626,67
-- PR Client 1,33 ms -- 8,07 1.43 1622253,33 118644

Second message

Test Mumble Client Execution time PID %CPU %MEM VSZ RSS
First -- -- -- --- --- - -
-- Orginal Client 13 ms 8650 6.8 1.4 1597788 117412
-- PR Client 13 ms 7903 8.2 1.4 1597716 116820
Second -- -- -- --- --- - -
-- Orginal Client 14 ms 8787 8.7 1.4 1597724 116492
-- PR Client 14 ms 9123 5.9 1.5 1597864 121536
Third -- -- -- --- --- - -
-- Orginal Client 15 ms 9669 7.4 1.5 1597828 121544
-- PR Client 14 ms 9332 7.1 1.4 1597724 117444
Summary -- -- -- --- --- - -
-- Orginal Client 14 ms -- 7.63 1.43 1597780 118482,67
-- PR Client 13,67 ms -- 7,07 1.43 1597768 118600

Third message

Test Mumble Client Execution time PID %CPU %MEM VSZ RSS
First -- -- -- --- --- - -
-- Orginal Client 39 ms 8707 10.1 1.4 1597820 119036
-- PR Client 34 ms 7940 5.6 1.4 1671516 118664
Second -- -- -- --- --- - -
-- Orginal Client 40 ms 8787 6.2 1.4 1597652 116612
-- PR Client 19 ms 9123 7.0 1.4 1597768 117608
Third -- -- -- --- --- - -
-- Orginal Client 39 ms 9669 9.8 1.4 1597404 117376
-- PR Client 19 ms 9332 5.9 1.4 1597780 117752
Summary -- -- -- --- --- - -
-- Orginal Client 39,33 ms -- 8,7 1.4 1597625,33 117674,67
-- PR Client 24 ms -- 6,61 1.4 1622354,67 118008

Summary of the messages

Test Mumble Client Execution time PID %CPU %MEM VSZ RSS
First -- -- -- --- --- - -
-- Orginal Client 1 ms -- 7.27 1.4 1597353,33 117626,67
-- PR Client 1,33 ms -- 8,07 1.43 1622253,33 118644
Second -- -- -- --- --- - -
-- Orginal Client 14 ms -- 7.63 1.43 1597780 118482,67
-- PR Client 13,67 ms -- 7,07 1.43 1597768 118600
Third -- -- -- --- --- - -
-- Orginal Client 39,33 ms -- 8,7 1.4 1597625,33 117674,67
-- PR Client 24 ms -- 6,61 1.4 1622354,67 118008
Summary -- -- -- --- --- - -
-- Orginal Client 18,11 ms -- 7,87 1.41 1597586,22 117928,0033
-- PR Client 13 ms -- 7,25 1.42 1614125,33 118417,33

@SuperNascher SuperNascher force-pushed the SuperNascher:message_size branch from e72a029 to 3d082c8 Aug 8, 2016

@SuperNascher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2016

Sorry that I post the results today, but my first objective was to test the changes on a 1GZ ARM device, but the compilation for the device took to long, so I have used my PC.

Here are the test results of sending the message (<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p> x 30) several times.

Used terms:
ET = Execution time in ms
VSZ = Virtual Memory Size in KiB (Kibibyte)
RSS = Resident Set Size, the non-swapped physical memory that a task has used in KB (Kilobytes)

Mumble PR Client: The Client with this Pull Request
Mumble Client: The dev client with this commit: e7ff17b

Mumble PR Client
10 messages:
ET (ms) %CPU %MEM VSZ (KiB) RSS (KB)
1 30 7,6 1,2 1576908 103200
2 21 8 1,2 1576908 103264
3 21 8,2 1,2 1576908 103280
4 22 8,6 1,2 1576908 103360
5 22 9 1,2 1576908 103424
6 22 9,2 1,2 1576908 103432
7 21 9,7 1,2 1576908 103440
8 21 10 1,2 1576908 103452
9 21 10,3 1,2 1576908 103464
10 21 10,7 1,2 1576908 103484
Summary 22,20 9,13 1,20 1.576.908,00 103.380,00
VSZ: 1.539,95 MiB
RSS: 103,38 MB
20 messages:
ET (ms) %CPU %MEM VSZ (KiB) RSS (KB)
1 29 10,8 1,2 1576908 101152
2 21 11,4 1,2 1576908 101220
3 21 12 1,2 1576908 101232
4 22 12,4 1,2 1576908 101312
5 21 13 1,2 1576908 101368
6 21 13,6 1,2 1576908 101380
7 22 14,2 1,2 1576908 101396
8 21 14,8 1,2 1576908 101408
9 21 12,8 1,2 1576908 101420
10 22 12,8 1,2 1576908 101432
11 22 13,3 1,2 1576908 101444
12 21 13,6 1,2 1576908 101564
13 21 14,1 1,2 1576908 101896
14 22 14,6 1,2 1576908 102200
15 22 15,1 1,2 1576908 102508
16 22 15,6 1,2 1577216 102828
17 22 16,6 1,2 1577528 103156
18 21 17,1 1,2 1577808 103524
19 21 17,6 1,2 1578084 103844
20 22 18,1 1,3 1580336 105112
Summary 21,85 14,18 1,21 1.577.229,60 102.069,80
VSZ: 1.540,26 MiB
RSS: 102,07 MB
30 messages:
ET (ms) %CPU %MEM VSZ (KiB) RSS (KB)
1 30 9,5 1,3 1576908 105616
2 21 9,8 1,3 1576908 105680
3 22 10,3 1,3 1576908 105684
4 21 10,8 1,3 1576908 105768
5 22 11,3 1,3 1576908 105832
6 22 11,8 1,3 1576908 105840
7 21 12,1 1,3 1576908 105852
8 21 12,6 1,3 1576908 105864
9 21 13,1 1,3 1576908 105872
10 22 13,6 1,3 1576908 105884
11 21 14,1 1,3 1576908 105908
12 21 14,5 1,3 1576908 105916
13 22 15,1 1,3 1576908 106264
14 22 15,6 1,3 1576908 106584
15 22 16,1 1,3 1576908 106876
16 21 16,5 1,3 1577232 107244
17 21 17,1 1,3 1577536 107560
18 22 17,5 1,3 1577816 107936
19 22 18 1,3 1578100 108284
20 22 18,5 1,3 1580376 109540
21 21 19 1,3 1580376 109632
22 21 19,6 1,3 1580376 109652
23 22 20,1 1,3 1580376 109744
24 21 17,7 1,3 1580376 109764
25 21 18,1 1,3 1580376 109836
26 21 18,5 1,3 1580376 109904
27 22 19 1,3 1580376 109960
28 21 19,4 1,3 1580376 110012
29 22 19,8 1,3 1580376 110136
30 22 20,4 1,3 1645916 110168
Summary 21,77 15,65 1,3 1.580.466,00 107.627,07
VSZ: 1.543,42 MiB
RSS: 107,63 MB
Mumble Client
10 messages:
ET (ms) %CPU %MEM VSZ (KiB) RSS (KB)
1 35 4,9 1.2 1576908 102060
2 35 5,1 1.2 1576908 102124
3 35 5,4 1,2 1576908 102132
4 35 5,6 1,2 1576908 102140
5 35 5,8 1,2 1576908 102156
6 35 6,1 1,2 1576908 102172
7 35 6,4 1,2 1576908 102180
8 34 6,6 1,2 1576908 102192
9 35 6,8 1,2 1576908 102204
10 35 7,1 1.2 1576908 102212
Summary 34,90 5,98 1,20 1.576.908,00 102.157,20
VSZ: 1.539,95 MiB
RSS: 102,16 MB
20 messages:
ET (ms) %CPU %MEM VSZ (KiB) RSS (KB)
1 35 9,6 1,2 1576908 103720
2 35 10,3 1,2 1576908 103784
3 35 11 1,2 1576908 103792
4 35 11,8 1,2 1576908 103808
5 34 12,5 1,2 1576908 103816
6 36 13,1 1,2 1576908 103828
7 35 13,8 1,2 1576908 103844
8 35 14,6 1,2 1576908 103852
9 35 15,3 1,2 1576908 103860
10 35 15,8 1,2 1576908 103892
11 35 16,6 1,2 1576908 103900
12 35 17,5 1,2 1576908 103908
13 34 18,1 1,2 1576908 103916
14 35 18,8 1,2 1576908 103932
15 36 19,5 1,2 1576908 103940
16 36 20,1 1,2 1576908 104060
17 35 21,1 1,2 1576908 104068
18 35 21,8 1,2 1576908 104072
19 36 19,4 1,2 1576908 104080
20 35 20 1,2 1576908 104084
Summary 35,10 16,04 1,20 1.576.908,00 103.907,80
VSZ: 1.539,95 MiB
RSS: 103,91 MB
30 messages:
ET (ms) %CPU %MEM VSZ (KiB) RSS (KB)
1 36 8,8 1,2 1576908 101672
2 35 9,4 1,2 1576908 101736
3 35 10 1,2 1576908 101748
4 35 10,5 1,2 1576908 101760
5 35 11,1 1,2 1576908 101780
6 34 11,7 1,2 1576908 101788
7 35 12,2 1,2 1576908 101796
8 34 12,8 1,2 1576908 101804
9 34 13,4 1,2 1576908 101816
10 35 12,3 1,2 1576908 101828
11 35 12,8 1,2 1576908 101852
12 34 13,5 1,2 1576908 101860
13 34 14 1,2 1576908 101880
14 35 14,5 1,2 1576908 101896
15 36 15 1,2 1576908 102076
16 35 15,6 1,2 1576908 102324
17 35 16,1 1,2 1576908 102336
18 35 16,7 1,2 1576908 102340
19 35 17,2 1,2 1576908 102356
20 35 17,7 1,2 1576908 102356
21 35 18,3 1,2 1576908 102356
22 35 18,8 1,2 1576908 102368
23 35 19,3 1,2 1576908 102368
24 35 19,8 1,2 1576908 102368
25 34 20,5 1,2 1576908 102368
26 35 21 1,2 1576908 102376
27 35 21,5 1,2 1576908 102376
28 35 22,1 1,2 1576908 102376
29 34 20,1 1,2 1576908 102376
30 35 20,6 1,2 1576908 102376
Summary 34,83 15,58 1,20 1.576.908,00 102.090,40
VSZ: 1.539,95 MiB
RSS: 102,09 MB

Summary

ET (ms) %CPU %MEM VSZ (KiB) RSS (KB)
10 messages:
Mumble PR Client 22,20 9,13 1,20 1.576.908,00 103.380,00
Mumble Client 34,90 5,98 1,20 1.576.908,00 102.157,20
20 messages:
Mumble PR Client 21,85 14,18 1,21 1.577.229,60 102.069,80
Mumble Client 35,10 16,04 1,20 1.576.908,00 103.907,80
30 messages:
Mumble PR Client 21,77 15,65 1,3 1.580.466,00 107.627,07
Mumble Client 34,83 15,58 1,20 1.576.908,00 102.090,40
@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 28, 2016

LGTM

@mkrautz mkrautz merged commit 13772c1 into mumble-voip:master Aug 28, 2016

@SuperNascher SuperNascher deleted the SuperNascher:message_size branch Aug 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.