brndnmtthws / conky

Light-weight system monitor for X, Wayland (sort of), and other things, too
https://conky.cc
GNU General Public License v3.0
7.31k stars 620 forks source link

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

Closed Enrico204 closed 3 weeks ago

Enrico204 commented 4 months ago

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"'
Caellian commented 4 months ago

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.

Enrico204 commented 4 months ago

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 commented 4 months ago

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

Enrico204 commented 4 months ago

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 :-)