Hello Han,

Han Boetes <hboetes@gmail.com> wrote:
> While reviewing word.c I noticed two memory leaks triggered by 
> transposing words with M-t.
> 
> First, in grabword(), the old *word buffer is never freed before 
> asprintf() overwrites the pointer on each iteration. This leaks every 
> previously allocated buffer except the last one.
> 
> Second, in transposeword(), word2 is set to NULL before it is freed, 
> making the free(word2) at the end of the function a no-op.
> 
> Both leaks are confirmed by valgrind, they trigger on every successful 
> M-t invocation, no OOM required.

Thank you!  Both diffs looks fine to me.  While reviewing thought, I
noticed that hitting asprintf() just to build a word char-by-char seems
a bit expensive and I couldn't resist simplifing a bit the code.

what do you think of this version of the change to grabword()?  It
should avoid the leak while also hammering malloc a bit less.  8 is
completely random, and we could even recalloc() to shrink down to the
right size to be pedantic at the end.

Thanks,
Omar Polo

diff -s /home/op/w/mg
path + /home/op/w/mg (staged changes)
commit - bb4e7911d276b83dbf547938ad8bc12ff94c2d0e
blob - 2409bb433a4c1188dd61963704bde2fe2299b736
blob + eda3354d5ba775ee974830541ef10d5a97fbde97
--- word.c
+++ word.c
@@ -203,17 +203,22 @@ transposeword(int f, int n)
 int
 grabword(char **word)
 {
-	int c;
+	size_t len = 0, cap = 0;
+	char *t;
 
 	while (inword() == TRUE) {
-		c = lgetc(curwp->w_dotp, curwp->w_doto);
-		if (*word == NULL) {
-			if (asprintf(word, "%c", c) == -1)
+		if (cap == 0 || len == cap - 1) {
+			t = recallocarray(*word, cap, cap + 8, 1);
+			if (t == NULL) {
+				free(*word);
+				*word = NULL;
 				return (errno);
-		} else {
-			if (asprintf(word, "%s%c", *word, c) == -1)
-				return (errno);
+			}
+			cap += 8;
+			*word = t;
 		}
+
+		(*word)[len++] = lgetc(curwp->w_dotp, curwp->w_doto);
 		(void)forwdel(FFRAND, 1);
 	}
 	if (*word == NULL)