hvisage / metalog

metalog is a syslog replacement that allows regular expresion matching
GNU General Public License v2.0
36 stars 11 forks source link

Incorrect neg_regex handling? #17

Closed auouymous closed 1 year ago

auouymous commented 1 year ago

The code at https://github.com/hvisage/metalog/blob/master/src/metalog.c#L1208 suggests a single regex match or single neg_regex mismatch will cause them all to pass the message. Shouldn't a single neg_regex match set regex_result = 0 and break out of the loop, causing all regex conditions to fail? Otherwise multiple neg_regex lines can't be used unless all match.

--- a/src/metalog.c
+++ b/src/metalog.c
@@ -1217,8 +1217,9 @@ static int processLogLine(const int logcode,
                 } else {
                     if (pcre2_match(this_regex->regex,
                                     (PCRE2_SPTR)info, (PCRE2_SIZE)info_len,
-                                    0, 0, match_data, NULL) < 0) {
-                        regex_result = 1;
+                                    0, 0, match_data, NULL) >= 0) {
+                        regex_result = 0;
+                        break;
                     }
                 }
                 this_regex++;

And program_neg_regex should probably do the same thing.

auouymous commented 1 year ago

A non-match for neg_regex must set regex_result to non-zero so the condition passes.

--- a/src/metalog.c
+++ b/src/metalog.c
@@ -1217,8 +1217,11 @@ static int processLogLine(const int logcode,
                 } else {
                     if (pcre2_match(this_regex->regex,
                                     (PCRE2_SPTR)info, (PCRE2_SIZE)info_len,
-                                    0, 0, match_data, NULL) < 0) {
-                        regex_result = 1;
+                                    0, 0, match_data, NULL) >= 0) {
+                        regex_result = 0;
+                        break;
+                    } else {
+                        regex_result = 1;
                     }
                 }
                 this_regex++;

This patch fixes the main issue but I'm not sure what to do when there are positive and negative rules and neither matches, which is a problem with or without the patch. It shouldn't pass because the positive didn't match, but the negative not matching causes it to pass. Swapping these rules produces the same result.

regex = "no match"
neg_regex = "no match"

A matching negative should cause the condition to fail, but should the non-matching negative also cause it to pass in the presence of non-matching positives? Or should more complex logic be added so it only passes if there are no positive rules or one or more positive rules matches?

auouymous commented 1 year ago

The following patch fails immediately if a neg_regex matches, and fails if none of the regex match, even with a non-matching neg_regex. This logic makes sense because you'd expect at least one matching regex to pass, unless there are no regex in the condition.

--- a/src/metalog.c
+++ b/src/metalog.c
@@ -1212,19 +1212,25 @@ static int processLogLine(const int logcode,
                     if (pcre2_match(this_regex->regex,
                                     (PCRE2_SPTR)info, (PCRE2_SIZE)info_len,
                                     0, 0, match_data, NULL) >= 0) {
+                        /* pass, unless a neg_regex matches */
                         regex_result = 1;
+                    } else if (regex_result != 1) {
+                        /* fail, unless another regex matches */
+                        regex_result = -1;
                     }
                 } else {
                     if (pcre2_match(this_regex->regex,
                                     (PCRE2_SPTR)info, (PCRE2_SIZE)info_len,
-                                    0, 0, match_data, NULL) < 0) {
-                        regex_result = 1;
+                                    0, 0, match_data, NULL) >= 0) {
+                        /* fail immediately */
+                        goto nextblock;
                     }
+                    /* pass, unless a non-matching regex is found or another neg_regex matches */
                 }
                 this_regex++;
                 nb_regexes--;
             } while (nb_regexes > 0);
-            if (regex_result == 0) {
+            if (regex_result == -1) {
                 goto nextblock;
             }
         }