* [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer
@ 2013-09-29 6:08 Michael Forney
2013-09-29 6:55 ` Rich Felker
2013-09-29 17:40 ` Rich Felker
0 siblings, 2 replies; 3+ messages in thread
From: Michael Forney @ 2013-09-29 6:08 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer
2013-09-29 6:08 [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer Michael Forney
@ 2013-09-29 6:55 ` Rich Felker
2013-09-29 17:40 ` Rich Felker
1 sibling, 0 replies; 3+ messages in thread
From: Rich Felker @ 2013-09-29 6:55 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer
2013-09-29 6:08 [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer Michael Forney
2013-09-29 6:55 ` Rich Felker
@ 2013-09-29 17:40 ` Rich Felker
1 sibling, 0 replies; 3+ messages in thread
From: Rich Felker @ 2013-09-29 17:40 UTC (permalink / raw)
To: musl
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(-)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-09-29 17:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-29 6:08 [PATCH] getgr_r: Reserve space for gr_mem's NULL terminator in buffer Michael Forney
2013-09-29 6:55 ` Rich Felker
2013-09-29 17:40 ` Rich Felker
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).