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.
autofs-5.1.3 - fix unset tsd group name handling
From: Ian Kent <raven@xxxxxxxxxx>
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@xxxxxxxxxx>
Reported-by: Donald Buczek <buczek@xxxxxxxxxxxxx>
---
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
