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

xdo_search: fix improper memfrees, pre-compile regexes optimization #407

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Cycatz
Copy link

@Cycatz Cycatz commented Oct 17, 2022

The patch was originally from @Lekensteyn at this commit in #36, I merged it with HEAD and also made some changes.

The commit fixes #258, which is causing by freeing uninitialized regex_t objects due to short-circuit evaluation.

if (!compile_re(search->title, &title_re) \
    || !compile_re(search->winclass, &class_re) \
    || !compile_re(search->winclassname, &classname_re) \
    || !compile_re(search->winrole, &role_re) \
    || !compile_re(search->winname, &name_re)) {

  regfree(&title_re);
  regfree(&class_re);
  regfree(&classname_re);
  regfree(&name_re);
  regfree(&role_re);

  return False;
}

The regex pre-compiled solution initializes the content of these regex_t objects to zero using memset so that regfrees don't crash.

In addition, the origin patch also resolves the issue of repeated error messages by compiling regex_t objects at start. These are all done by @Lekensteyn, awesome!

Aside from resolving merging conflicts, I add regex_t struct for role, rename xdg_search_re to xdg_regex_t for style consistency and apply the changes that constantify regex_t* where applicable from @Lekensteyn.

BTW, I remove the whitespaces removal code changes, I think it should be made in other commit. If there is any need, I can help with that.

Original author: Peter Wu <[email protected]>
Modified by: Cycatz <[email protected]>

If any of compile_re statements fails, all uninitialized regex_t objects
are freed due to short-circuit evaluation, which causing segmentation faults.
Now they are initialized to zero with memset and besides that,
all regexes are now pre-compiled before iterating through all windows.

(as a side-effect of the optimization: return early when a regex fails
to compile).

Other changes: constantify regex_t* where applicable.
@aldot
Copy link
Contributor

aldot commented May 4, 2023

This is one of those cases where one would usually use a goto, but given we don't have a single goto in there ATM, i doubt this would be well received.

I think i would prefer the much simpler tweak, although it could rightfully be considered looking ugly and annoyingly repetitive:

diff --git a/xdo_search.c b/xdo_search.c
index 9842927..258a9d7 100644
--- a/xdo_search.c
+++ b/xdo_search.c
@@ -210,11 +210,10 @@ static int _xdo_match_window_pid(const xdo_t *xdo, Window window, const int pid)
 static int compile_re(const char *pattern, regex_t *re) {
   int ret;
   if (pattern == NULL) {
-    regcomp(re, "^$", REG_EXTENDED | REG_ICASE);
-    return True;
+    ret = regcomp(re, "^$", REG_EXTENDED | REG_ICASE);
+  } else {
+    ret = regcomp(re, pattern, REG_EXTENDED | REG_ICASE);
   }
-
-  ret = regcomp(re, pattern, REG_EXTENDED | REG_ICASE);
   if (ret != 0) {
     fprintf(stderr, "Failed to compile regex (return code %d): '%s'\n", ret, pattern);
     return False;
@@ -240,12 +239,28 @@ static int check_window_match(const xdo_t *xdo, Window wid,
   regex_t role_re;
 
 
-  if (!compile_re(search->title, &title_re) \
-      || !compile_re(search->winclass, &class_re) \
-      || !compile_re(search->winclassname, &classname_re) \
-      || !compile_re(search->winrole, &role_re) \
-      || !compile_re(search->winname, &name_re)) {
+  if (!compile_re(search->title, &title_re)) {
+    return False;
+  }
+  if (!compile_re(search->winclass, &class_re)) {
+    regfree(&title_re);
 
+    return False;
+  }
+  if (!compile_re(search->winclassname, &classname_re)) {
+    regfree(&title_re);
+    regfree(&class_re);
+
+    return False;
+  }
+  if (!compile_re(search->winrole, &role_re)) {
+    regfree(&title_re);
+    regfree(&class_re);
+    regfree(&classname_re);
+
+    return False;
+  }
+  if (!compile_re(search->winname, &name_re)) {
     regfree(&title_re);
     regfree(&class_re);
     regfree(&classname_re);

Just a thought.
PS: beware, i didn't even try to compile the hunk above, this is for illustrative purpose only!
PPS: the compile_re hunk should be committed separately, "Check regcomp even for NULL pattern" or the like. Not strictly needed but it looks odd NOT to check if "^$" compiled. Unlikely, but just for good measure.
HTH,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault on "failed to compile regex"
2 participants