Review Request 126931: Fix crash when importing cmake tests whose arguments contain ')'

[Andrew Coles]

> On Feb. 1, 2016, 12:13 p.m., Milian Wolff wrote:
> > can you add a unit test for this please?

If I could I would.  The affected function is hidden within a .cpp file, so I can't call it directly from a test harness outside of this - I'd have to correctly initialise an object of the class type, which is beyond what I can do right now given I have zero familiarity with the relevant code.

I've attached a file which could be used as the basis of a unit test, if you can cause:

`importTestSuites(Path("/path/to/the/file/attached"));`

..to execute, it will seg fault as promised.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126931/#review91847
-----------------------------------------------------------


On Feb. 1, 2016, 4:01 p.m., Andrew Coles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126931/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2016, 4:01 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Bugs: 358454
>     http://bugs.kde.org/show_bug.cgi?id=358454
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> In projectmanagers/cmake/cmakeimportjsonjob.cpp, there's a regular expression that matches 'add_test' lines from CTestTestfile.cmake files, but it stops reading at the first ) character:
> 
> `const QRegularExpression rx("add_test *\((.+?) (.*?)\)");`
> 
> If the argument list for the add_test line contains brackets, for instance:
> 
> add_test(some-test "/some/path" "(foo)" "bar")
> 
> ...then this causes a segfault a few lines further down - the second rx capture is not a well-formed argument list.
> 
> This patch changes the regexp to only stop when ) is at the end of the line:
> 
> `const QRegularExpression rx("add_test *\((.+?) (.*?)\) *$");`
> 
> It also allows for spaces between the ')' and the end of the line, to be consistent with allowing extra spacing earlier on after add_test.  I'm not sure if this matters in practice but is harmless enough.
> 
> 
> Diffs
> -----
> 
>   projectmanagers/cmake/cmakeimportjsonjob.cpp 0502be7 
> 
> Diff: https://git.reviewboard.kde.org/r/126931/diff/
> 
> 
> Testing
> -------
> 
> It can now import CTestTestfile.cmake files from my projects without crashing.  I haven't tested it with other people's cmake build trees.
> 
> 
> File Attachments
> ----------------
> 
> This will cause a seg fault in importTestSuites
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/02/01/e3bfaea9-27e8-4d39-881b-aaf493981d30__CTestTestfile.cmake
> 
> 
> Thanks,
> 
> Andrew Coles
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160201/021dd2dc/attachment.html>