Icinga2: String#substr has inconsistent / erroneous behaviour

[seventh-chord]

This check in string-script.cpp should check whether the first argument is > self.GetLength() rather than >= self.GetLength(). As it currently stands, one can not take a substring of the empty string (i.e. "".substring(0, 0) fails in icinga-script) or take an empty substring starting at one past the last character in a string (i.e. "nice".substr(4, 0) also fails). This is inconsistent with the behaviour of std::string::substr, which is what your implementation is built upon. I also believe that it is inconsistent with what most people would expect the function to do. Your documentation does not say anything about how String#substr handles out-of-bounds cases, which though not completly related, doesn't help the issue.

[dnsmichi]

Thanks. Can you create a PR which includes some unit tests to better illustrate the issue?

[seventh-chord]

I'm sorry, I do not have a development environment for icinga2 set up, and setting it up just to add a unit test for a simple issue seems overkill (You have like 10 dependencies that need to be installed, and probably at least one of those will mess up my current cpp dev env on windows). I'd be happy to give more detailed examples of the issues though, if you would like.

[dnsmichi]

Yep, some more examples on the cases you've encountered, would be nice. This helps anyone trying to reproduce, debug and fix the problem.

[seventh-chord]

Some examples. Note that this is using icinga version 2.10.4-1 on debian, but looking at the source code (see initial links in the issue) this bug should still be present.

root@my-server ~ $ icinga2 console
Icinga 2 (version: r2.10.4-1)
# NB Spacing added for clarity

<1> => "hamburger".substr(0, 7)
"hamburg"

<2> => "something".substr(9, 0)
       ^^^^^^^^^^^^^^^^^^^^^^^^
Error while evaluating expression: String index is out of range

<3> => "something".substr(10, 0)
       ^^^^^^^^^^^^^^^^^^^^^^^^
Error while evaluating expression: String index is out of range

<4> => "".substr(0, 0)
       ^^^^^^^^^^^^^^^
Error while evaluating expression: String index is out of range

Overview of what icinga2 does, compared to what I would expect it to do.

|Input|Actual output|Expected output|
|-|-|-|
|"hamburger".substr(0,7)|"hamburg"|"hamburg"|
|"something".substr(9,0)|Error|""|
|"something".substr(10,0)|Error|Error|
|"".substr(0,0)|Error|""|

[dnsmichi]

Thanks. @htriem I will come to you when this issue is on the task list, for now assigning this to allow better filtering.