#520 Bug: Wrong condition evaluation in MacrosManager::EvalCondition

[bluehazzard]

i have found a bug in the function
wxString MacrosManager::EvalCondition(const wxString& in_cond, const wxString& true_clause, const wxString& false_clause, ProjectBuildTarget* target):

sdk/macrosmanager.cpp:616

wxString MacrosManager::EvalCondition(const wxString& in_cond, const wxString& true_clause, const wxString& false_clause, ProjectBuildTarget* target)
{
    enum condition_codes {EQ = 1, LT = 2, GT = 4, NE = 8};

    wxString cond(in_cond);
    wxString result;

    ReplaceMacros(cond, target, true);

    if (!m_RE_IfSp.Matches(in_cond))
        return false_clause;

    wxString cmpToken(m_RE_IfSp.GetMatch(in_cond, 3).Strip(wxString::both));
    wxString left(m_RE_IfSp.GetMatch(in_cond, 2).Strip(wxString::both));
    wxString right(m_RE_IfSp.GetMatch(in_cond, 4).Strip(wxString::both));

The function argument "in_cond" is copied to cond. Then in "cond" all macros are replaced, but then left and right are extracted from the non macro replaced "in_cond" variable. This is wrong. There the "cond" variable should be used:

    wxString cmpToken(m_RE_IfSp.GetMatch(cond, 3).Strip(wxString::both));
    wxString left(m_RE_IfSp.GetMatch(cond, 2).Strip(wxString::both));
    wxString right(m_RE_IfSp.GetMatch(cond, 4).Strip(wxString::both));

I will look if I can post a patch

[bluehazzard]

here is the tested patch
I am not quite sure what

if (!m_RE_IfSp.Matches(in_cond))
        return false_clause;

does

[bluehazzard]

Does something speaks against this patch?

[Teodor Petrov]

No, idea... you're the master of the MacroManager...

[Teodor Petrov]

Just keep in mind that we're close to a release and committing risky stuff is not really a good idea.

[bluehazzard]

Ok some more information:

if (!m_RE_IfSp.Matches(in_cond))
        return false_clause;

Prevents recursion. It stops the evaluation if the in condition is an "if" statement

I tried to apply and test this patch. Without this patch the implementation this function is completely useless. And the replace macros function is not used.

But also with this patch it is completely useless, because the repexp evaluation of the "if" condition:

\$if\(([^)]*)\) *(\{([^}]*)\})(\{([^}]*)\})?

can not evaluate something like

$if($(TARGET_NAME)==Debug){[[Log(_(\"true\"))]]}{[[Log(_(\"false\"))]]}

as far as i can tell it fails because of the first appearing ")" for the variable expansion
So if you can not evaluate variables you can not use the whole "if" function?

And as far as i can tell this can not be solved with regexp provided by wxWidgets (POSIX flavor).
So we have to parse this without regexp?

[bluehazzard]

Ok, i made a parser that i think works fine....
But now i face a other problem. In MacrosManager::EvalCondition

    if (!m_RE_IfSp.Matches(cond))
        return false_clause;

    wxString cmpToken(m_RE_IfSp.GetMatch(cond, 3).Strip(wxString::both));
    wxString left(m_RE_IfSp.GetMatch(cond, 2).Strip(wxString::both));
    wxString right(m_RE_IfSp.GetMatch(cond, 4).Strip(wxString::both));

the condition is parsed with regexp. The regexp string is for this is

[^=!<>]+|(([^=!<>]+)[ ]*(=|==|!=|>|<|>=|<=)[ ]*([^=!<>]+))

and as far as i can tell (i do not know regexp, but i tested it with https://regex101.com/ and it does not match the cmpToken. And in codeblocks it is broken totally. It matches for the left condition the whole string and all other are empty. The test string is

Debug==Debug

(the motivation for fixing this if condition is to make it easy to make post build steps platform independent)

[bluehazzard]

Ok, the right regexp is this:

(([^=!<>]+)[ ]*(=|==|!=|>|<|>=|<=)[ ]*([^=!<>]+))

[Morten MacFly]

RegEx has many flavours. One is the wx one. This is probably not the one of regex101. Bei careful! We have for such purposes a regex plugin. this one.

[bluehazzard]

Thank you! I did not knew we have this ^^
I tested it with the plugin and both make no difference on Linux and work the same.
But yesterday i debugged the code on windows with the first regexp and it did not work.
Can it be that there are some differences between windows and linux?
For what is this part needed?

[^=!<>]+|

[Morten MacFly]

Should read use this one...

[bluehazzard]

Ok, i made a patch to hopefully fix all this.
You can test this patch with the following line in the scripting console:

ReplaceMacros(_("$if($(TARGET_NAME)==Debug){[[Log(_(\"{true\"))]]}{[[Log(_(\"}}{false\"))]]}"));

if the selected target is named "Debug" it should print out "{true" in the log window.
The parenthesis are for testing purpose.

I currently try to write some test scripts for the replace macro function in squirrel, but i have found a bug in the scripting engine, that makes some problems...

As always comments and critique is needed and welcome.

[bluehazzard]

to create proper test scripts bug https://sourceforge.net/p/codeblocks/tickets/812/ is blocking this

[bluehazzard]

A other test script blocking bug: https://sourceforge.net/p/codeblocks/tickets/817/

[bluehazzard]

Here is a new version of the patch. During writing tests i have found some bugs, that are fixed in this version....

[bluehazzard]

With the fix in ticket 817 this are the used tests

[Teodor Petrov]

1.3 doesn't look like it will compile for wx2.8

[Teodor Petrov]

1. You have to write tests which try to break extractStringBetweenParentesis? It is complex and I think it doesn't work as expected. The first loop for example could detect an open brace in a string. Something like this 'bla bla "this is broken ( and would return ") end of string' would result in ' and would return"'
2. Why do you do resultString += , why don't you just record the start and end positions and do results = s.substring(start, end)?
3. The header file seems missing you're adding some methods but they are not added to the class. Generally it is better to add helper function which doesn't depend on this to be static local functions. Because this makes it posible to remove them without the need of recompilation
4. isString ^= true could be replaced with the more common isString = !isString.

[bluehazzard]

Where should i write the tests? In squirrel?

The first loop for example could detect an open brace in a string. Something like this 'bla bla "this is broken ( and would return ") end of string' would result in ' and would return"'

i do not understand this... Can you use code tags?
Are you talking about this:

bla bla "this is broken ( and would return ") end of string

would return

 and would return "

well it is exactly what it should return.... This parser should not be too complex... The syntax of the macro replacing is quite simple, and if the user does not follow this syntax then it is broken and won't work as expected...
1) After the $if should follow a ( and no obscure string combination... maybe some spaces but thats it...
2) The string in $if( can have some wired things, like scripting or other macros, but i think for non fancy scripting this should work as expected (at least the easy tests i wrote work) i do not expect the user to use extended scripts. For this he can use the scripting engine and not that $if syntax....
3) The string between the two {} could be more complex, with bash scripts and so on... Here you possibly could be right, that we should also look and escape ' for bash scripts? Any good fancy test cases here?

[bluehazzard]

Found some nasty string that fails the test:

 PS1="\$(awk '/MemFree/{print \$2}' /proc/meminfo) prompt > "

[Teodor Petrov]

Ignore the comment about tests. I guess you're handling this with scripts.

What do you mean by complex? Now you have two loops and you can achieve the same thing using a single loop and also you can handle the case you depicted correctly. The result you say is correct is not. This is the exact problem I have in mind and you should handle it. People would write random things here. Either on purpose or by mistake. Your parser should do the best job it can to help them diagnose the problem!

3) I'm not talking about bash escapes at all.

p.s. I cannot be bothered to format post here... it is so annoying.

[bluehazzard]

Ok i updated my code, and i think i worked with your suggestions. I will post a patch tomorrow.
I have found that if i have this:

ReplaceMacros(_("$if(true){ PS1=\"\\$(awk '/MemFree/{print \\$2}' /proc/meminfo) prompt > \"}{false}"))

the resulting string is exactly what i expect:

 PS1="\$(awk '/MemFree/{print \$2}' /proc/meminfo) prompt > "

but then the

m_RE_Unix.Matches(buffer)

in macrosmanager.cpp:622 kicks in and thinks

$(awk

is a variable with name AWK and will replace it. It seems that this regexp is also not working as intended...

[bluehazzard]

Ok, here are the patches... comments welcome.