PATCH: utils.c: Fix use of uninitialized memory in metafy().

PATCH: utils.c: Fix use of uninitialized memory in metafy().

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

---
Hello,

While running the tests with valgrind I noticed an use of
uninitialized memory in metafy().

The following patch should fix it, but I don't know the details
of this code, so please check it before applying the patch.

The problem is the *e != '\0' in the next if, once e == buf +
len, *e points after buf.

Regards,
Simon

 Src/utils.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Src/utils.c b/Src/utils.c
index 0db9c30..eb71aab 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -3980,10 +3980,14 @@ metafy(char *buf, int len, int heap)
 	for (e = buf, len = 0; *e; len++)
 	    if (imeta(*e++))
 		meta++;
-    } else
+    } else {
 	for (e = buf; e < buf + len;)
 	    if (imeta(*e++))
 		meta++;
+	/* go to last byte of buf */
+	if (len > 0)
+	    e--;
+    }
 
     if (meta || heap == META_DUP || heap == META_HEAPDUP || *e != '\0') {
 	switch (heap) {
-- 
1.8.4.4.12.gcc59366.dirty

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: PATCH: utils.c: Fix use of uninitialized memory in metafy().

[Peter Stephenson]

On Wed, 27 Nov 2013 18:45:16 +0100
Simon Ruderich <simon@ruderich.org> wrote:
> While running the tests with valgrind I noticed an use of
> uninitialized memory in metafy().
>
> The following patch should fix it, but I don't know the details
> of this code, so please check it before applying the patch.
> 
> The problem is the *e != '\0' in the next if, once e == buf +
> len, *e points after buf.

Hmm... I think the intention probably *is* to check if there's null
termination at "buf + len", on the assumption that the first "len" bytes
need metafying regardless.  So if we've got only len valid bytes, not
null-terminated (or null-terminated by accident because the next byte
that isn't actually valid for the allocation happens to be null), we've
got no way of knowing this given the current interface.  That's not
actually stated explicitly but the comment above does mention len+1
for copying, implying len doesn't include the termination.

It looks like either we've got to improve the interface, which is a lot
of work, or always copy when we're give a length, which is inefficient.
I'd be tempted to do the latter for now.

pws

Re: PATCH: utils.c: Fix use of uninitialized memory in metafy().

[Bart Schaefer]

On Nov 27,  6:07pm, Peter Stephenson wrote:
} 
} Hmm... I think the intention probably *is* to check if there's null
} termination at "buf + len", on the assumption that the first "len" bytes
} need metafying regardless.  So if we've got only len valid bytes, not
} null-terminated (or null-terminated by accident because the next byte
} that isn't actually valid for the allocation happens to be null), we've
} got no way of knowing this given the current interface.

Does it actually matter?  The only reason for (*e != 0) as far as I can
tell is to be sure we've actually done (*e = '\0') at the very end of
the whole thing [comment: "... unchanged (a terminating null character
is appended to buf if necessary)"].

Can't we just move the *e = '\0' outside the "if" body and skip the test
in the condition?

All tests still pass with the following:

diff --git a/Src/utils.c b/Src/utils.c
index 0db9c30..c6d178c 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -3985,7 +3985,7 @@ metafy(char *buf, int len, int heap)
 	    if (imeta(*e++))
 		meta++;
 
-    if (meta || heap == META_DUP || heap == META_HEAPDUP || *e != '\0') {
+    if (meta || heap == META_DUP || heap == META_HEAPDUP) {
 	switch (heap) {
 	case META_REALLOC:
 	    buf = zrealloc(buf, len + meta + 1);
@@ -4028,8 +4028,8 @@ metafy(char *buf, int len, int heap)
 		meta--;
 	    }
 	}
-	*e = '\0';
     }
+    *e = '\0';
     return buf;
 }
 

Re: PATCH: utils.c: Fix use of uninitialized memory in metafy().

[Peter Stephenson]

On Wed, 27 Nov 2013 10:54:09 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Nov 27,  6:07pm, Peter Stephenson wrote:
> } 
> } Hmm... I think the intention probably *is* to check if there's null
> } termination at "buf + len", on the assumption that the first "len" bytes
> } need metafying regardless.  So if we've got only len valid bytes, not
> } null-terminated (or null-terminated by accident because the next byte
> } that isn't actually valid for the allocation happens to be null), we've
> } got no way of knowing this given the current interface.
> 
> Does it actually matter?  The only reason for (*e != 0) as far as I can
> tell is to be sure we've actually done (*e = '\0') at the very end of
> the whole thing [comment: "... unchanged (a terminating null character
> is appended to buf if necessary)"].
> 
> Can't we just move the *e = '\0' outside the "if" body and skip the test
> in the condition?

Seems reasonable --- it requires the problem Simon was seeing to be in a
case that's requesting reallocation, else that assignment is going to
cause problems, but if it does cause problems we need to change the
caller.

pws

Re: PATCH: utils.c: Fix use of uninitialized memory in metafy().

[Bart Schaefer]

On Nov 27,  8:26pm, Peter Stephenson wrote:
} Subject: Re: PATCH: utils.c: Fix use of uninitialized memory in metafy().
}
} On Wed, 27 Nov 2013 10:54:09 -0800
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > On Nov 27,  6:07pm, Peter Stephenson wrote:
} > } 
} > } ....  So if we've got only len valid bytes, not
} > } null-terminated (or null-terminated by accident because the next byte
} > } that isn't actually valid for the allocation happens to be null), we've
} > } got no way of knowing this given the current interface.
} > 
} > Does it actually matter?  The only reason for (*e != 0) as far as I can
} > tell is to be sure we've actually done (*e = '\0') at the very end of
} > the whole thing [comment: "... unchanged (a terminating null character
} > is appended to buf if necessary)"].
} > 
} > Can't we just move the *e = '\0' outside the "if" body and skip the test
} > in the condition?
} 
} Seems reasonable --- it requires the problem Simon was seeing to be in a
} case that's requesting reallocation, else that assignment is going to
} cause problems, but if it does cause problems we need to change the
} caller.

No, the problem Simon was seeing must in fact occur only in cases that
have no metacharacters and are NOT requesting reallocation, otherwise
the short-circuiting behavior of logical-or would never get as far as
testing (*e != '\0').

But (in the current code) if the test succeeds, then we enter the block
and execute *e = '\0', and if the test fails then *e == '\0' must be
true.  The only case in which assigning '\0' could be a (new) problem is
one where the byte at buf[len] is already zero but is somehow in a part
of memory to which we aren't allowed to write.

Or where (len > 0 && *e && e != buf+len) but I don't see how that could
happen either.  We could throw in a DPUTS if you are worried.

Simon's valgrind report wasn't a pointer out-of-bounds error, it was an
uninitialized memory error.

Re: PATCH: utils.c: Fix use of uninitialized memory in metafy().

[Peter Stephenson]

On Wed, 27 Nov 2013 17:19:37 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Simon's valgrind report wasn't a pointer out-of-bounds error, it was an
> uninitialized memory error.

Yes, that's what I've been ignoring.

pws