Bug report #13919

QGIS server security patch: random crashes and performance regression

Added by Andreas Neumann over 4 years ago. Updated over 4 years ago.

Status:Closed
Priority:Severe/Regression
Assignee:Nyall Dawson
Category:QGIS Server
Affected QGIS version:master Regression?:No
Operating System:Linux Ubuntu 64bit Easy fix?:No
Pull Request or Patch supplied:No Resolution:
Crashes QGIS or corrupts data:No Copied to github as #:21941

Description

With the 422abbd - PR#2056 I get random crashes when vector data is involved (from Postgis).

There is also a quite large performance regression - many more complex projects with lots of Postgis layers are 2-3 times slower than before this patch. This can be reproduced with any project that has lots of Postgis layers.

I will try to create a backtrace from the crashes - if someone can help me create one.

Associated revisions

Revision 752f6bd1
Added by Nyall Dawson over 4 years ago

Fix classes which violate rule of three, by implementing required
copy/= operators or making them private

This revealed (and fixes) some issues, including a potential crash
using server access control (refs #13919), and a potential crash with
diagrams

Revision 69ce5599
Added by Nyall Dawson over 4 years ago

Fix some oddities in server access control and bindings (refs #13919)

Revision 0446f507
Added by Nyall Dawson over 4 years ago

Fix slow filter rect requests with server python plugins (refs #13919)

Revision 90a4ae80
Added by Nyall Dawson over 4 years ago

Fix #13919, don't reset layer subsetStrings when server python plugins
are enabled

History

#1 Updated by Andreas Neumann over 4 years ago

Maybe the issues can be related to Martin Dobias' comment on the issue?:

@sbrunner This is unfortunately very dangerous to introduce pointer to original QgsVectorLayer and has to be avoided.

The correct approach would be that a custom feature filter provider stores any data that it may need. This change may otherwise introduce race conditions and crashes - and others may mistakenly try to use mLayer for more functionality, leading to even more problems.

Please could you update QgsFeatureFilterProvider class so that it does not use QgsVectorLayer directly? There is mLayerID member variable in QgsMapLayerRenderer which may be used to identify a layer if necessary.

#2 Updated by Jürgen Fischer over 4 years ago

  • Assignee set to Stéphane Brunner

#3 Updated by Stéphane Brunner over 4 years ago

Andreas Neumann I think that's not related on what Martin Dobias say....

Can you specify what you are doing?
Are you using QGIS server?
Is it also visible on QGIS client?

#4 Updated by Stéphane Brunner over 4 years ago

In all case I will have a look on what Martin Dobias say :-)

#5 Updated by Andreas Neumann over 4 years ago

I get a crash in my Apache log - randomly. And the WMS client gets a corrupted image - tested in both QGIS Desktop or OpenLayers as a client.

[Mon Nov 30 09:17:58.227308 2015] [core:error] [pid 15465] [client 10.63.238.200:51961] End of script output before headers: qgis_mapserv.fcgi, referer: http://gisbrowser/maps/leitungskataster/leitungskataster?format=image/png;%20mode=8bit&visibleLayers=Swisscom,Gas,Wasser,Fernw%C3%A4rme,Elektro,Abwasser,%C3%9Cbersicht%20Vektor25,Nomenklatur,Geb%C3%A4ude,Grenzen,Einzelobjekte,Rohrleitungen,Bodenbedeckung,Liegenschaften%20Fl%C3%A4che,Einzelobjekte%20Unterst%C3%A4nde,Einzelobjekte%20Unterirdische%20Geb%C3%A4ude&fullColorLayers=Orthofoto%202010%20(25cm),Orthofoto%202008%20(10cm)&startExtent=692000,241500,700100,249000&maxExtent=692000,241500,700100,249000&searchtables=abwasser.suchtabelle
[Mon Nov 30 09:17:59.308226 2015] [fcgid:error] [pid 14799] mod_fcgid: process /home/www/cgi/qgis_mapserv.fcgi(16196) exit(communication error), get signal 11, possible coredump generated

#6 Updated by Stéphane Brunner over 4 years ago

Hello Andreas,

I'm having a look, I will get some new when I have something to test :-)

CU
Stéphane

#7 Updated by Stéphane Brunner over 4 years ago

@Andreas Neumann

I just implements what's Martin Dobias speaks here:
https://github.com/sbrunner/QGIS/tree/access-control-fix

Can you test it?

#8 Updated by Andreas Neumann over 4 years ago

Hi Stéphane,

I hope you had a good Christmas vacation!

I tested https://github.com/sbrunner/QGIS/commit/6d56cba465b3612c7e685d724e0e8ecc55c5e1bc

Unfortunately, I still get the crashes and the slowness ;-(

I could help debug QGIS Desktop issues, but I am not good at helping debug QGIS sever issues ...

Perhaps we need another QGIS dev to help us review the issue?

Let me know, if you have any ideas.

Andreas

#9 Updated by Andreas Neumann over 4 years ago

Note that all the project I tested with QGIS server use Postgis as data source - if that helps.

#10 Updated by Stéphane Brunner over 4 years ago

I don't succeed to reproduce it, do you have an example?
Do you use CGI or FCGID?
Witch version of postgres, postgis do you use?

#11 Updated by Nyall Dawson over 4 years ago

  • Status changed from Open to Feedback

Please test with current master, possibly fixed

#12 Updated by Andreas Neumann over 4 years ago

Finally I had the chance to install QGIS server on a different (non-productive) server.

I have good and bad news.

The good news: the crashes are gone
The bad news: it is still considerably slower than before your patch, Stéphane.

I will ask Nyall if he is available to examine the issue why is so much slower.

#13 Updated by Nyall Dawson over 4 years ago

  • Status changed from Feedback to In Progress
  • Assignee changed from Stéphane Brunner to Nyall Dawson

Tracked this down. Working on a fix now.

#14 Updated by Andreas Neumann over 4 years ago

Hi Stéphane,

Nyall will soon provide a fix for my performance issue.

The issue was around incorrectly clearing the filters that do server side filtering. Then the server fetched all features and filtered locally - as I understand - rather than filtering on the PostgreSQL server.

BTW: Nyall praises your excellent and exhaustive unit tests! They also helped to uncover additional issues unrelated to this issue.

BTWII: the crash issue had to do with the inverted polygon renderer. Not sure if this was caused by your patch or some other issue.

Thanks to both of you!
Andreas

#15 Updated by Stéphane Brunner over 4 years ago

Thanks @Nyall for your investigations, and @Andreas Neumann for your tests and patience :-)

#16 Updated by Nyall Dawson over 4 years ago

  • Status changed from In Progress to Closed

Also available in: Atom PDF