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

[Bug]: if_match parser doesn't handle comparison of strings containing ! #1988

Closed
Enrico204 opened this issue Jul 15, 2024 · 4 comments · Fixed by #2064
Closed

[Bug]: if_match parser doesn't handle comparison of strings containing ! #1988

Enrico204 opened this issue Jul 15, 2024 · 4 comments · Fixed by #2064
Labels
bug related to incorrect existing implementation of some functionality good first issue straightforward enough for first-time contributors to be able to implement themselves text related to `conky.text` variables, their parsing or implementation

Comments

@Enrico204
Copy link

What happened?

The Wi-Fi ESSID is not escaped correctly, and it is confusing if_match. The execution continues, but the first code block is always matched regardless of the values (see log and config).

I tested both with the Debian version (1.18) and the latest version using AppImage (1.21.4), the problem is the same.

Version

1.21.4

Which OS/distro are you seeing the problem on?

Debian

Conky config

conky.text = [[${color white}
WiFi:
${if_up wlp59s0}${if_match "${wireless_essid wlp59s0}" == "off/any"}${else}Wi-Fi    ↑ ${upspeed wlp59s0} - ↓ ${downspeed wlp59s0}
        ${wireless_essid wlp59s0} (${wireless_channel wlp59s0}) ${wireless_link_qual_perc wlp59s0}%
        ${addrs wlp59s0}
$endif$endif
]]

Stack trace

No response

Relevant log output

conky: failed to parse compare string '"FRITZ!Box 7520 HI" == "off/any"'
conky: compare failed for expression '"FRITZ!Box 7520 HI" == "off/any"'
@Enrico204 Enrico204 added bug related to incorrect existing implementation of some functionality triage issue that hasn't been verified, categorized or acknowledged yet labels Jul 15, 2024
@Caellian Caellian added priority: low issue that's not encountered often or hard to reproduce networking related to network information reporting and removed triage issue that hasn't been verified, categorized or acknowledged yet labels Jul 15, 2024
@Caellian
Copy link
Collaborator

Marking as low priority because #1001 needs to be addressed first (work on #1879). My guess is that this has to do with if_match instead of networking.

Can you provide an example which doesn't use the wireless_essid variable, even if echoing from a file with similar content? Doing so would allow us to unlink the issues.

@Caellian Caellian added the text related to `conky.text` variables, their parsing or implementation label Jul 15, 2024
@Enrico204
Copy link
Author

You are right, the same problem exists when using exec:

conky.text = [[${color white}
TEST: ${if_match "${exec echo FRITZ!Box 7520 HI}" == "off/any"}a${else}b$endif
]]

This configuration produces the same error. Is there any special meaning for the exclamation mark in if_match?

@Caellian Caellian removed priority: low issue that's not encountered often or hard to reproduce networking related to network information reporting labels Jul 15, 2024
@Caellian
Copy link
Collaborator

Looked into the code, I can confirm it's a bug in the following two functions in algebra.cc:

int find_match_op(const char *expr) {
  unsigned int idx;

  for (idx = 0; idx < strlen(expr); idx++) {
    switch (expr[idx]) {
      case '=':
      case '!':
        if (expr[idx + 1] != '=') { return -1; }
        /* falls through */
      case '<':
      case '>':
        return idx;
    }
  }
  return -1;
}

int get_match_type(const char *expr) {
  int idx;
  const char *str;

  if ((idx = find_match_op(expr)) == -1) { return -1; }
  str = expr + idx;

  if (*str == '=' && *(str + 1) == '=') { return OP_EQ; }
  if (*str == '!' && *(str + 1) == '=') { return OP_NEQ; }
  if (*str == '>') {
    if (*(str + 1) == '=') { return OP_GEQ; }
    return OP_GT;
  }
  if (*str == '<') {
    if (*(str + 1) == '=') { return OP_LEQ; }
    return OP_LT;
  }
  return -1;
}

get_match_type finds the first occurrence of ! via find_match_op and then tries matching it to !=, given that the text can contain arbitrary number of ! before an actual !=, or even the whole != expression, the function get_match_type is faulty because it assumes the comparison is after the first !. FRITZ! routers are very common consumer grade routers provided by ISPs so this was only the most likely failure point.

It should instead parse the first value (string or float), skip the value, skip whitespace, then parse operator, fail if wrong operator or end of text (first string not closed), skip whitespace, parse second value/operand, fail if types not comparable (handle string to number conversion), return comparison result.

Workaround: use exec and get the same information from shell, pipe through sed s/!//g

@Caellian Caellian added the good first issue straightforward enough for first-time contributors to be able to implement themselves label Jul 15, 2024
@Caellian Caellian changed the title [Bug]: Wi-Fi ESSID not escaped [Bug]: if_match parser doesn't handle comparison of strings with ! Jul 15, 2024
@Caellian Caellian changed the title [Bug]: if_match parser doesn't handle comparison of strings with ! [Bug]: if_match parser doesn't handle comparison of strings containing ! Jul 15, 2024
@Enrico204
Copy link
Author

Thank you for the triage!

I will use the workaround for now. Unfortunately, I don't think I can help in the development for now, but I am available for any test/experimenti :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug related to incorrect existing implementation of some functionality good first issue straightforward enough for first-time contributors to be able to implement themselves text related to `conky.text` variables, their parsing or implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants