* [PATCH] Fix usage of uninitialized pointer
@ 2017-05-30 13:35 Donald Buczek
2017-05-31 0:27 ` Ian Kent
0 siblings, 1 reply; 6+ messages in thread
From: Donald Buczek @ 2017-05-30 13:35 UTC (permalink / raw)
To: raven, autofs; +Cc: Donald Buczek
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.
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 {
+ 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);
return;
free_tsv_group:
- if (tsv->group)
- free(tsv->group);
+ free(tsv->group);
+free_gr_tmp:
+ if (gr_tmp)
+ free(gr_tmp);
free_tsv_home:
free(tsv->home);
free_tsv_user:
--
2.4.1
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix usage of uninitialized pointer
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
0 siblings, 1 reply; 6+ messages in thread
From: Ian Kent @ 2017-05-31 0:27 UTC (permalink / raw)
To: Donald Buczek, autofs
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.
>
> 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.
> return;
>
> free_tsv_group:
> - if (tsv->group)
> - free(tsv->group);
> + free(tsv->group);
> +free_gr_tmp:
> + if (gr_tmp)
> + free(gr_tmp);
> free_tsv_home:
> free(tsv->home);
> free_tsv_user:
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix usage of uninitialized pointer
2017-05-31 0:27 ` Ian Kent
@ 2017-05-31 1:37 ` Ian Kent
2017-05-31 14:22 ` Donald Buczek
0 siblings, 1 reply; 6+ messages in thread
From: Ian Kent @ 2017-05-31 1:37 UTC (permalink / raw)
To: Donald Buczek; +Cc: autofs
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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix usage of uninitialized pointer
2017-05-31 1:37 ` Ian Kent
@ 2017-05-31 14:22 ` Donald Buczek
2017-05-31 20:28 ` Donald Buczek
0 siblings, 1 reply; 6+ messages in thread
From: Donald Buczek @ 2017-05-31 14:22 UTC (permalink / raw)
To: Ian Kent; +Cc: autofs
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?
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;
--
Donald Buczek
buczek@molgen.mpg.de
Tel: +49 30 8413 1433
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix usage of uninitialized pointer
2017-05-31 14:22 ` Donald Buczek
@ 2017-05-31 20:28 ` Donald Buczek
2017-06-01 9:13 ` Ian Kent
0 siblings, 1 reply; 6+ messages in thread
From: Donald Buczek @ 2017-05-31 20:28 UTC (permalink / raw)
To: Ian Kent; +Cc: autofs
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.
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;
>
>
--
Donald Buczek
buczek@molgen.mpg.de
Tel: +49 30 8413 1433
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix usage of uninitialized pointer
2017-05-31 20:28 ` Donald Buczek
@ 2017-06-01 9:13 ` Ian Kent
0 siblings, 0 replies; 6+ messages in thread
From: Ian Kent @ 2017-06-01 9:13 UTC (permalink / raw)
To: Donald Buczek; +Cc: autofs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-01 9:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.