From: Ian Kent <raven@themaw.net>
To: Donald Buczek <buczek@molgen.mpg.de>
Cc: autofs@vger.kernel.org
Subject: Re: [PATCH] Fix usage of uninitialized pointer
Date: Thu, 01 Jun 2017 17:13:33 +0800 [thread overview]
Message-ID: <1496308413.2580.6.camel@themaw.net> (raw)
In-Reply-To: <350fd28e-8e61-24b0-f2e4-38a4476c8108@molgen.mpg.de>
On Wed, 2017-05-31 at 22:28 +0200, Donald Buczek wrote:
> On 31.05.2017 16:22, Donald Buczek wrote:
> > On 05/31/17 03:37, Ian Kent wrote:
> > > On Wed, 2017-05-31 at 08:27 +0800, Ian Kent wrote:
> > > > On Tue, 2017-05-30 at 15:35 +0200, Donald Buczek wrote:
> > > > > In the error path after a getgrgid_r failure (e.g. when a unnamed gid
> > > > > was used), the pointer tsv->group was left unitialized. Still the tsv
> > > > > was given to pthread_setspecific(key_thread_stdenv_vars,...) and the
> > > > > consumers used and freed the uninitialized pointer.
> > > >
> > > > Umm ... ok, I'll check ... good catch.
> > >
> > > I think this bug will warrant another release.
> > >
> > > > > 2017-05-29T18:16:11+02:00 rofl automount[14749]: attempting to
> > > > > mount
> > > > > entry
> > > > > /package/twiki
> > > > > 2017-05-29T18:16:11+02:00 rofl automount[14749]:
> > > > > set_tsd_user_vars:
> > > > > failed
> > > > > to get group info from getgrgid_r
> > > > > 2017-05-29T18:16:11+02:00 rofl automount[14749]: mounted
> > > > > /package/twiki
> > > > > 2017-05-29T18:16:11+02:00 rofl systemd[1]: automount.service:
> > > > > main
> > > > > process
> > > > > exited, code=dumped, status=6
> > > > > 2017-05-29T18:16:12+02:00 rofl systemd[1]: automount.service
> > > > > holdoff
> > > > > time
> > > > > over, scheduling restart.
> > > > > 2017-05-29T18:16:12+02:00 rofl systemd[1]: Unit automount.service
> > > > > entered
> > > > > failed state.
> > > > > 2017-05-29T18:16:12+02:00 rofl automount[17936]: Starting
> > > > > automounter
> > > > > version 5.1.3, master map auto.master
> > > > >
> > > > > [May29 18:16] traps: automount[18234] general protection
> > > > > ip:7f8b025c324a
> > > > > sp:7f8b0049a508 error:0 in libc-2.19.so[7f8b02541000+1a2000]
> > > > >
> > > > > Handle the error by not calling pthread_setspecific. Clean up
> > > > > and return instead.
> > > > > ---
> > > > > lib/mounts.c | 21 ++++++++++++---------
> > > > > 1 file changed, 12 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/lib/mounts.c b/lib/mounts.c
> > > > > index ce6a60a..fe1a6cd 100644
> > > > > --- a/lib/mounts.c
> > > > > +++ b/lib/mounts.c
> > > > > @@ -1552,28 +1552,31 @@ void set_tsd_user_vars(unsigned int logopt,
> > > > > uid_t
> > > > > uid,
> > > > > gid_t gid)
> > > > > }
> > > > > no_group:
> > > > > - if (status || !pgr)
> > > > > + if (status || !pgr) {
> > > > > error(logopt, "failed to get group info from getgrgid_r");
> > > > > - else {
> > > >
> > > > Extra braces, I leave these out (when I can) on single statement clauses
> > > > deliberately and always try to put the single statement as the first
> > > > branch of
> > > > the if.
> > > >
> > > > > + goto free_gr_tmp;
> > > > > + } else {
> > > > > tsv->group = strdup(gr.gr_name);
> > > > > - if (!tsv->group)
> > > > > + if (!tsv->group) {
> > > > > error(logopt, "failed to malloc buffer for group");
> > > > > + goto free_gr_tmp;
> > > > > + }
> > > > > }
> > > > > - if (gr_tmp)
> > > > > - free(gr_tmp);
> > > > > -
> > > > > status = pthread_setspecific(key_thread_stdenv_vars, tsv);
> > > > > if (status) {
> > > > > error(logopt, "failed to set stdenv thread var");
> > > > > goto free_tsv_group;
> > > > > }
> > > > > -
> > > > > + if (gr_tmp)
> > > > > + free(gr_tmp);
> > > >
> > > > But this doesn't do what I intended.
> > > >
> > > > What I want to do is set the thread specific standard variables even
> > > > if the
> > > > group name can't be obtained.
> > > >
> > > > It looks like I've made an assumption elsewhere that if the tsd is
> > > > set then
> > > > all
> > > > the variables have valid values, I'd rather fix that than do this.
> > >
> > > I would prefer to do this instead.
> > >
> > > Could you check this resolves the problem you have seen please.
> >
> > Sorry, the patch doesn't apply, because it is line wrapped and contains
> > utf8 unicode non breaking space characters. Can I pull it?
>
>
> Never mind, edited it in by hand.
>
> I can confirm, that the bug is fixed.
>
> Test procedure:
>
> run
>
> valgrind --leak-check=full daemon/automount -f -v
>
> in one shell. Run
>
> perl -e '$(=65533;$)=65533;system("ls /project/SOME_AUTOMOUNT_PATH")'
>
> in another.
>
> Result from master (release_5.1.3) are several valgrind warnings and
> death by segv:
>
> set_tsd_user_vars: failed to get group info from getgrgid_r
> ==18686== Use of uninitialised value of size 8
> ==18686== at 0x4C2AAF2: strlen (vg_replace_strmem.c:454)
> ==18686== by 0x619FF8D: strdup (strdup.c:41)
> [...]
> ==18686== Invalid read of size 1
> ==18686== at 0x4C2AAF2: strlen (vg_replace_strmem.c:454)
> ==18686== by 0x619FF8D: strdup (strdup.c:41)
> ==18686== by 0x13A18D: macro_addvar (macros.c:305)
> [...]
> ==18686== Address 0x0 is not stack'd, malloc'd or (recently) free'd
> ==18686== Process terminating with default action of signal 11
> (SIGSEGV): dumping core
> ==18686== Access not within mapped region at address 0x0
> ==18686== at 0x4C2AAF2: strlen (vg_replace_strmem.c:454)
> ==18686== by 0x619FF8D: strdup (strdup.c:41)
> ==18686== by 0x13A18D: macro_addvar (macros.c:305)
> ==18686== by 0x12F1E4: do_macro_addvar (mounts.c:368)
>
> Result with you patch on top: No errors (aside from expected
> "set_tsd_user_vars: failed to get group info from getgrgid_r") and a
> mounted directory.
>
> Didn't try to expand the variables in a map, though.
>
> Thumbs up from my side.
Thanks for doing this.
>
> Donald
>
>
>
> >
> > Donald
> >
> >
> > >
> > > autofs-5.1.3 - fix unset tsd group name handling
> > >
> > > From: Ian Kent <raven@themaw.net>
> > >
> > > Commit 1a64a6bbc5 changed set_tsd_user_vars() to set the thread specific
> > > values even if the group name could not be obtained.
> > >
> > > But the structure holding the values was not initialized on allocation
> > > so the group field might not be NULL when no group name is available.
> > >
> > > Also the macro addition and removal functions didn't handle setting a
> > > macro name with a NULL value.
> > >
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > Reported-by: Donald Buczek <buczek@molgen.mpg.de>
> > > ---
> > > lib/macros.c | 27 +++++++++++++--------------
> > > lib/mounts.c | 1 +
> > > 2 files changed, 14 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/lib/macros.c b/lib/macros.c
> > > index ff9ba89..30dbcdb 100644
> > > --- a/lib/macros.c
> > > +++ b/lib/macros.c
> > > @@ -281,19 +281,21 @@ macro_addvar(struct substvar *table, const char
> > > *str, int
> > > len, const char *value
> > > }
> > > if (lv) {
> > > - char *this = malloc(strlen(value) + 1);
> > > - if (!this) {
> > > - lv = table;
> > > - goto done;
> > > + char *this = NULL;
> > > +
> > > + if (value) {
> > > + this = malloc(strlen(value) + 1);
> > > + if (this)
> > > + strcpy(this, value);
> > > }
> > > - strcpy(this, value);
> > > - free(lv->val);
> > > + if (lv->val)
> > > + free(lv->val);
> > > lv->val = this;
> > > if (lv != table)
> > > lv = table;
> > > } else {
> > > struct substvar *new;
> > > - char *def, *val;
> > > + char *def, *val = NULL;
> > > def = strdup(str);
> > > if (!def) {
> > > @@ -302,18 +304,15 @@ macro_addvar(struct substvar *table, const char
> > > *str, int
> > > len, const char *value
> > > }
> > > def[len] = '\0';
> > > - val = strdup(value);
> > > - if (!val) {
> > > - lv = table;
> > > - free(def);
> > > - goto done;
> > > - }
> > > + if (value)
> > > + val = strdup(value);
> > > new = malloc(sizeof(struct substvar));
> > > if (!new) {
> > > lv = table;
> > > free(def);
> > > - free(val);
> > > + if (val)
> > > + free(val);
> > > goto done;
> > > }
> > > new->def = def;
> > > diff --git a/lib/mounts.c b/lib/mounts.c
> > > index ce6a60a..0b38bd8 100644
> > > --- a/lib/mounts.c
> > > +++ b/lib/mounts.c
> > > @@ -1463,6 +1463,7 @@ void set_tsd_user_vars(unsigned int logopt,
> > > uid_t uid,
> > > gid_t gid)
> > > error(logopt, "failed alloc tsv storage");
> > > return;
> > > }
> > > + memset(tsv, 0, sizeof(struct thread_stdenv_vars));
> > > tsv->uid = uid;
> > > tsv->gid = gid;
> >
> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
prev parent reply other threads:[~2017-06-01 9:13 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-30 13:35 [PATCH] Fix usage of uninitialized pointer Donald Buczek
2017-05-31 0:27 ` Ian Kent
2017-05-31 1:37 ` Ian Kent
2017-05-31 14:22 ` Donald Buczek
2017-05-31 20:28 ` Donald Buczek
2017-06-01 9:13 ` Ian Kent [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1496308413.2580.6.camel@themaw.net \
--to=raven@themaw.net \
--cc=autofs@vger.kernel.org \
--cc=buczek@molgen.mpg.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).