Bug 43051 - italc 3.0 segfault in screenshot()
italc 3.0 segfault in screenshot()
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: iTALC
UCS@school 4.1 R2
Other Linux
: P5 normal (vote)
: UCS@school 4.1 R2 vXXX
Assigned To: Florian Best
Sönke Schwardt-Krummrich
: interim-3
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-25 11:44 CET by Florian Best
Modified: 2016-12-12 13:10 CET (History)
4 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 3: Will affect average number of installed domains
How will those affected feel about the bug?: 4: A User would return the product
User Pain: 0.343
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number: 2016112421000151
Bug group (optional): External feedback, Usability
Max CVSS v3 score:


Attachments
patch (1.50 KB, patch)
2016-11-25 12:30 CET, Florian Best
Details | Diff
Reproducer (1.20 KB, text/x-python)
2016-11-29 17:01 CET, Florian Best
Details

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Florian Best univentionstaff 2016-11-25 11:50:18 CET
The segfault occurs during creation of a screenshot of a teachers computer.
Comment 2 Florian Best univentionstaff 2016-11-25 12:30:56 CET
Created attachment 8242 [details]
patch
Comment 3 Florian Best univentionstaff 2016-11-25 16:29:07 CET
#0  qRed (rgb=<error reading variable: Cannot access memory at address 0x9785000>) at ../../../../include/QtGui/../../src/gui/painting/qrgb.h:58
#1  write_jpeg_image (image=..., device=0x7f1029d3ba6c, sourceQuality=-1) at ../../../gui/image/qjpeghandler.cpp:660
#2  0x00007f1086207472 in QImageWriter::write (this=0x7f1029cf6880, image=...) at image/qimagewriter.cpp:606
Comment 4 Florian Best univentionstaff 2016-11-29 17:01:57 CET
Created attachment 8254 [details]
Reproducer

I have a reproducible way to trigger this segfault:
* Change the screen resolution of a client during the "watch"-mode.

I extracted a single script to trigger the segfault. See attachment.

The cause is in lib/src/ItalcVncConnection.cpp:
The method ItalcVncConnection::hookNewClient(rfbClient *cl) does basically this:
ItalcVncConnection * t = (ItalcVncConnection *) rfbClientGetClientData(cl, 0);
if (t->m_frameBuffer)
    delete [] t->m_frameBuffer;
t->m_frameBuffer = new uint8_t[size];
cl->frameBuffer = t->m_frameBuffer;
memset(cl->frameBuffer, '\0', size);

→ So cl->frameBuffer gets replaces with a new buffer, the old buffer gets deleted.

We are using the image() method (which returns this->m_image) to receive an QImage "img". Then we are calling img->save(path, "JPG") and the segfault occurs.

The ItalcVncConnection::hookUpdateFB(rfbClient *cl) it the one who sets this->m_image to QImage img(cl->frameBuffer, cl->width, cl->height, QImage::Format_RGB32).

So cl->frameBuffer which gets deleted later in a concurrent thread is given to the QImage.

My proposed solution is to copy the image when setting it. I am currently unsure about the memory handling of C++/QT so I asked Tobias via mail about my patch:
diff --git a/lib/src/ItalcVncConnection.cpp b/lib/src/ItalcVncConnection.cpp
index 4fd55be..05749c6 100644
--- a/lib/src/ItalcVncConnection.cpp
+++ b/lib/src/ItalcVncConnection.cpp
@@ -449,7 +449,7 @@ void ItalcVncConnection::setImage( const QImage & img )
 {
 »   m_imgLock.lockForWrite();
 »   const QSize oldSize = m_image.size();
-»   m_image = img;
+»   m_image = img.copy(img.rect());
 »   m_imgLock.unlock();
 
 »   if( img.size() != oldSize )
Comment 5 Florian Best univentionstaff 2016-12-05 16:36:53 CET
ucs-school-umc-computerroom.yaml:
r74780 | YAML Bug #43044 Bug #43051

italc.yaml:
r74780 | YAML Bug #43044 Bug #43051

italc (2:2.0.25-15):
r74990 | Bug #43051: fix segfaults during screenshot creation
r74988 | Bug #43051: fix segfaults during screenshot creation
r74767 | Bug #43051: revert patch
r74762 | Bug #43044 / Bug #43051: fix segfaults

ucs-school-umc-computerroom (8.0.13-1):
r74778 | Bug #43051: enhance screenshot creation

ucs-school-italc.yaml:
r74780 | YAML Bug #43044 Bug #43051
Comment 7 Florian Best univentionstaff 2016-12-05 17:32:45 CET
New upstream commit: https://github.com/iTALC/italc/commit/039386fec2c6cf990ef17ab1a7ecc84bf05f3eea
Comment 8 Florian Best univentionstaff 2016-12-05 18:37:01 CET
(In reply to Florian Best from comment #7)
> New upstream commit:
> https://github.com/iTALC/italc/commit/
> 039386fec2c6cf990ef17ab1a7ecc84bf05f3eea

This commit is only compatible with QT 5.
Nevertheless all my pull requests are now accepted and we are again at the HEAD of the italc-2 branch for the backend italc master process.

italc (2:2.0.25-16):
r75004 | Bug #43051: fix segfaults during screenshot creation
Comment 9 Sönke Schwardt-Krummrich univentionstaff 2016-12-08 17:44:14 CET
OK: problem was reproducible
OK: code change
OK: functional check (problem is solved)
OK: packages built (ucs-school-umc-computerroom, italc, python-italc)
OK: advisories (applied small changes)
Comment 10 Sönke Schwardt-Krummrich univentionstaff 2016-12-09 11:05:26 CET
Addition:
No memory leak was observed with the "Reproducer" script. No increase in memory usage when requesting large amounts of screenshots or when frequently changing screen resolution. The process size remained constant.
Comment 11 Sönke Schwardt-Krummrich univentionstaff 2016-12-12 13:10:14 CET
UCS@school 4.1 R2 v9 has been released.

http://docs.software-univention.de/changelog-ucsschool-4.1R2v9-de.html