[PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer

[PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer

[Michael Forney]

Currently, the NULL terminator overlaps with the beginning of the line, causing
gr_name to always be the empty string.
---
As an aside, I don't understand why 32 is added to the size check. It looks
like the length is rounded down to a multiple of 16, so at most 15 extra bytes
will be needed. But even so, wouldn't it be better to check for exactly the
amount of space that will be used? Or is it not worth the additional temporary
variable?

 src/passwd/getgr_r.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/passwd/getgr_r.c b/src/passwd/getgr_r.c
index 234c901..3fe2e2b 100644
--- a/src/passwd/getgr_r.c
+++ b/src/passwd/getgr_r.c
@@ -26,14 +26,14 @@ static int getgr_r(const char *name, gid_t gid, struct group *gr, char *buf, siz
 	while (__getgrent_a(f, gr, &line, &len, &mem, &nmem)) {
 		if (name && !strcmp(name, gr->gr_name)
 		|| !name && gr->gr_gid == gid) {
-			if (size < len + nmem*sizeof(char *) + 32) {
+			if (size < len + (nmem+1)*sizeof(char *) + 32) {
 				rv = ERANGE;
 				break;
 			}
 			*res = gr;
 			buf += (16-(uintptr_t)buf)%16;
 			gr->gr_mem = (void *)buf;
-			buf += nmem*sizeof(char *);
+			buf += (nmem+1)*sizeof(char *);
 			memcpy(buf, line, len);
 			FIX(name);
 			FIX(passwd);
-- 
1.8.4

Re: [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer

[Rich Felker]

On Sat, Sep 28, 2013 at 11:08:46PM -0700, Michael Forney wrote:
> Currently, the NULL terminator overlaps with the beginning of the line, causing
> gr_name to always be the empty string.

Thanks. Fix committed.

> ---
> As an aside, I don't understand why 32 is added to the size check. It looks
> like the length is rounded down to a multiple of 16, so at most 15 extra bytes
> will be needed. But even so, wouldn't it be better to check for exactly the
> amount of space that will be used? Or is it not worth the additional temporary
> variable?

I'd have to look back and see if there's any good reason; offhand, I
agree with your analysis.

Rich

Re: [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer

[Rich Felker]

By the way, how did you come across the bugs you're reporting? Are you
testing software against musl, or directly reviewing the source? I'm
just curious since you've already found a couple important issues that
our existing methodologies have so far missed, and I'm hoping we can
stamp out a lot more bugs like this between now and the 1.0 release.

Rich



On Sat, Sep 28, 2013 at 11:08:46PM -0700, Michael Forney wrote:
> Currently, the NULL terminator overlaps with the beginning of the line, causing
> gr_name to always be the empty string.
> ---
> As an aside, I don't understand why 32 is added to the size check. It looks
> like the length is rounded down to a multiple of 16, so at most 15 extra bytes
> will be needed. But even so, wouldn't it be better to check for exactly the
> amount of space that will be used? Or is it not worth the additional temporary
> variable?
> 
>  src/passwd/getgr_r.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)