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

[processing][gdal] Do not escape formula because it's not a path! #6960

Merged
merged 1 commit into from
May 10, 2018

Conversation

luipir
Copy link
Contributor

@luipir luipir commented May 9, 2018

Description

numpy formula in gdal_calc processing algoritm is managed as a path and escaped in case having spaces. This escape generate triky error during gdal execution.
Just test the error setting the default formula from "A*2" to "A * 2".

fix promoted inside GeoMove [1] project for Cartolab [2] (A Coruña university)

[1] http://cartolab.udc.es/geomove/
[2] http://cartolab.udc.es

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@luipir
Copy link
Contributor Author

luipir commented May 9, 2018

Please @alexbruy can you give a look?

@nyalldawson
Copy link
Collaborator

Is this an issue on master too?

@luipir
Copy link
Contributor Author

luipir commented May 9, 2018

@nyalldawson should be, but gdalcalc is still not available as geoprocess in processing and I hadn't time to integrate it.

@nyalldawson
Copy link
Collaborator

@luipir ah, that's a shame -- i wasn't aware there was unported algorithms here

@luipir luipir changed the title avoid escape formula because it's not a path! [processing][gdal] Do not escape formula because it's not a path! May 9, 2018
@luipir
Copy link
Contributor Author

luipir commented May 9, 2018

confirmed the error should be the same on qgis3...the code is not changed.
No great experience writing tests for processing classes. In this case I would only test if the parsed --calc formula returned by getConsoleCommands is not changed.

@luipir
Copy link
Contributor Author

luipir commented May 9, 2018

@nyalldawson np I'm not really a processing man, I just realized that it was not available because a collegue here in CartoLab wasn't able to calc a SARVI index with any qgis version. They always have to test on any win version before to notify me any error.

@nyalldawson
Copy link
Collaborator

@luipir
For gdal/ogr algorithms the best way is to add tests like this: https://github.com/qgis/QGIS/blob/master/python/plugins/processing/tests/GdalAlgorithmsTest.py#L297

But that's for master, not 2.18.

@luipir
Copy link
Contributor Author

luipir commented May 9, 2018

For gdal/ogr algorithms the best way is to add tests like this: https://github.com/qgis/QGIS/blob/master/python/plugins/processing/tests/GdalAlgorithmsTest.py#L297

But that's for master, not 2.18

wonderfull.. I'll add test and pr to 3.x tomorrow. I just checked if there where tests on 2.18.

@alexbruy alexbruy merged commit 10aa92f into qgis:release-2_18 May 10, 2018
@alexbruy
Copy link
Contributor

Thanks.
BTW, why not use native calculator?

@luipir
Copy link
Contributor Author

luipir commented May 10, 2018

@alexbruy because crash qgis with with memory allocation problem with big images with any version and platform (SAGA has the same problem). I didn't check personally but I saw some tests. I can check personally to add more information about these crashes.
In general on win he message was "allocation error".

@luipir
Copy link
Contributor Author

luipir commented May 10, 2018

@nyalldawson I'll check if it would be possible to pass and evaluate dangerous formulas like A*2"; rm * without appropriate escaping. GDAL injection?

@luipir luipir deleted the gdalcalc_avoid_escape_formula branch December 17, 2018 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants