> if (p<*toys.optargs || p>toys.optargs[toys.optc-1]) not_argv();
>
> It would be nice to free this data ourselves because expr is close to
> $((1+2)) in the shell, and it would be nice if the shell could just
> internall call expr nofork in that case.
I attached to a patch to track the strings allocated and then free
them at the end.
(Note that there is NOT a "node" per argument ('struct value' which
which contains an int or string). The nodes are on the stack and used
destructively in the style of a *= b, a += b, etc. What we are
talking about is the strings that the nodes (may) point to. These are
almost always argv strings which don't need to be freed, but in the 2
cases I mentioned, we allocate them.
So now this is fixed, and I tested it with ASAN (with LLVM 3.8 which
can be downloaded easily). It easily finds the memory leak before
(only the regex test cases where we allocate memory fail, not every
test case) and confirms they pass after the patch.
ASAN also found another existing memory leak in the code! (not
calling regfree()) When I'm triaging the tests I'll definitely see
which ones pass under ASAN too.
Andy
From 1069d89210edbf2b16571fa905a4e30eebde9e80 Mon Sep 17 00:00:00 2001
From: Andy Chu <[email protected]>
Date: Wed, 16 Mar 2016 11:21:50 -0700
Subject: [PATCH 3/3] Track and free all strings allocated during 'expr'
evaluation.
Also fix a pre-existing memory leak in re() by calling regfree().
---
toys/pending/expr.c | 29 ++++++++++++++++++++++++++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/toys/pending/expr.c b/toys/pending/expr.c
index 71d5d19..4a49371 100644
--- a/toys/pending/expr.c
+++ b/toys/pending/expr.c
@@ -48,6 +48,7 @@ config EXPR
GLOBALS(
char* tok; // current token, not on the stack since recursive calls mutate it
+ struct arg_list *allocated; // list of strings allocated during evaluation
)
// Scalar value. If s != NULL, it's a string, otherwise it's an int.
@@ -69,6 +70,14 @@ void syntax_error(char *msg, ...) {
#define LONG_LONG_MAX_LEN 21
+// Keep track of an allocated string.
+void track_str(char* str) {
+ struct arg_list *node = xmalloc(sizeof(struct arg_list));
+ node->arg = str;
+ node->next = TT.allocated;
+ TT.allocated = node;
+}
+
// Get the value as a string.
void get_str(struct value *v, char** ret)
{
@@ -77,6 +86,7 @@ void get_str(struct value *v, char** ret)
else {
*ret = xmalloc(LONG_LONG_MAX_LEN);
snprintf(*ret, LONG_LONG_MAX_LEN, "%lld", v->i);
+ track_str(*ret); // free it later
}
}
@@ -118,9 +128,10 @@ static void re(char *target, char *pattern, struct value *ret)
xregcomp(&pat, pattern, 0);
if (!regexec(&pat, target, 2, m, 0) && m[0].rm_so == 0) { // match at pos 0
regmatch_t* g1 = &m[1]; // group capture 1
- if (pat.re_nsub > 0 && g1->rm_so >= 0) // has capture
+ if (pat.re_nsub > 0 && g1->rm_so >= 0) { // has capture
ret->s = xmprintf("%.*s", g1->rm_eo - g1->rm_so, target + g1->rm_so);
- else
+ track_str(ret->s); // free it later
+ } else
assign_int(ret, m[0].rm_eo);
} else { // no match
if (pat.re_nsub > 0) // has capture
@@ -128,6 +139,7 @@ static void re(char *target, char *pattern, struct value *ret)
else
assign_int(ret, 0);
}
+ regfree(&pat);
}
// 4 different signatures of operators. S = string, I = int, SI = string or
@@ -271,5 +283,16 @@ void expr_main(void)
if (ret.s) printf("%s\n", ret.s);
else printf("%lld\n", ret.i);
- exit(is_false(&ret));
+ int status = is_false(&ret);
+
+ // Free all the strings we allocated.
+ struct arg_list *h, *head = TT.allocated;
+ while (head) {
+ h = head;
+ head = head->next;
+ free(h->arg); // free the string
+ free(h); // free the node for tracking the string
+ }
+
+ exit(status);
}
--
1.9.1
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
