handling NULL params in sss_parse_name

[Pavel Reichl]

Hello,

sss_parse_name currently SIGSEGVs if any of output parameters (domain,
name) is a NULL. I think it may be useful to support passing NULL
arguments as not always both pieces of information are needed.

Please see attached patch.

PR

[Lukas Slebodnik]

On (26/01/14 13:02), Pavel Reichl wrote:
> Hello,
>
> sss_parse_name currently SIGSEGVs if any of output parameters (domain,
> name) is a NULL. I think it may be useful to support passing NULL
> arguments as not always both pieces of information are needed.
>
> Please see attached patch.
>
> PR
> From 1cbd8b48b1359bbba8315cd904b93643594dd0d3 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Sun, 26 Jan 2014 12:39:43 +0000
> Subject: [PATCH] utils: handling NULL params in sss_parse_name
>
> src/util/usertools.c | 48 ++++++++++++++++++++++++++----------------------
> 1 file changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/usertools.c b/src/util/usertools.c
> index c77aa7ce097d32d002bf7168bea91082b0e8e4dd..24665d21f0718f35c4844a359a324eba8b8ff8da 100644
> --- a/src/util/usertools.c
> +++ b/src/util/usertools.c
> @@ -327,31 +327,35 @@ int sss_parse_name(TALLOC_CTX *memctx,
> strnum = ret;
>
> result = NULL;
> ret = pcre_get_named_substring(re, orig, ovec, strnum, "name", &result);
> if (ret < 0  || !result) {
>    DEBUG(2, ("Name not found!\n"));
>    return EINVAL;
> if (name != NULL) {
>    result = NULL;
>    ret = pcre_get_named_substring(re, orig, ovec, strnum, "name", &result);
>    if (ret < 0  || !result) {
>        DEBUG(2, ("Name not found!\n"));
>        return EINVAL;
>    }
>    *name = talloc_strdup(memctx, result);
>    pcre_free_substring(result);
>    if (!*name) return ENOMEM;
> }
>
> *name = talloc_strdup(memctx, result);
>
> pcre_free_substring(result);
>
> if (!*name) return ENOMEM;
>
> result = NULL;
>
> ret = pcre_get_named_substring(re, orig, ovec, strnum, "domain", &result);
>
> if (ret < 0  || !result) {
>    DEBUG(4, ("Domain not provided!\n"));
>    *domain = NULL;
>
> } else {
>    /* ignore "" string */
>    if (*result) {
>        *domain = talloc_strdup(memctx, result);
>        pcre_free_substring(result);
>        if (!*domain) return ENOMEM;
>    } else {
>        pcre_free_substring(result);
> if (domain != NULL) {
>    result = NULL;
>    ret = pcre_get_named_substring(re, orig, ovec, strnum, "domain",
>                                   &result);
>    if (ret < 0  || !result) {
>        DEBUG(4, ("Domain not provided!\n"));
>       *domain = NULL;
>    } else {
>        /* ignore "" string */
>        if (*result) {
>            *domain = talloc_strdup(memctx, result);
>            pcre_free_substring(result);
>            if (!*domain) return ENOMEM;
>        } else {
>            pcre_free_substring(result);
>            *domain = NULL;
>        }
>   }
> }
>
> --
> 1.8.4.2

Can you also add '_' as prefix for output parameters into declaration.
We are not consistent in this and I cannot find this in coding style,
but it improves readability.

LS

[Jakub Hrozek]

On Mon, Jan 27, 2014 at 01:02:19PM +0100, Lukas Slebodnik wrote:
> On (26/01/14 13:02), Pavel Reichl wrote:
> > Hello,
> >
> > sss_parse_name currently SIGSEGVs if any of output parameters (domain,
> > name) is a NULL. I think it may be useful to support passing NULL
> > arguments as not always both pieces of information are needed.
> >
> > Please see attached patch.
> >
> > PR
>
> Can you also add '_' as prefix for output parameters into declaration.
> We are not consistent in this and I cannot find this in coding style,
> but it improves readability.
>
> LS

Right, this is not codified, but I prefer to prefix output variables,
too.

[Pavel Reichl]

[snip]
> Can you also add '_' as prefix for output parameters into declaration.
> We are not consistent in this and I cannot find this in coding style,
> but it improves readability.
>
> LS

Hello Lukas,

thank you for your comment. New patch implementing your suggestion is
attached.

PR

[Lukas Slebodnik]

On (27/01/14 13:43), Pavel Reichl wrote:
> [snip]
> > Can you also add '_' as prefix for output parameters into declaration.
> > We are not consistent in this and I cannot find this in coding style,
> > but it improves readability.
> >
> > LS
>
> Hello Lukas,
>
> thank you for your comment. New patch implementing your suggestion is
> attached.
>
> PR
> From 95560c8b33ad6b8efdce67b6098d5cdd88fb4129 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Sun, 26 Jan 2014 12:39:43 +0000
> Subject: [PATCH] utils: handling NULL params in sss_parse_name
>
> src/util/usertools.c | 50 +++++++++++++++++++++++++++-----------------------
> src/util/util.h      |  2 +-
> 2 files changed, 28 insertions(+), 24 deletions(-)

ACK

LS

[Jakub Hrozek]

On Mon, Jan 27, 2014 at 02:25:25PM +0100, Lukas Slebodnik wrote:
> On (27/01/14 13:43), Pavel Reichl wrote:
> > [snip]
> > > Can you also add '_' as prefix for output parameters into declaration.
> > > We are not consistent in this and I cannot find this in coding style,
> > > but it improves readability.
> > >
> > > LS
> >
> > Hello Lukas,
> >
> > thank you for your comment. New patch implementing your suggestion is
> > attached.
> >
> > PR
> > From 95560c8b33ad6b8efdce67b6098d5cdd88fb4129 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Sun, 26 Jan 2014 12:39:43 +0000
> > Subject: [PATCH] utils: handling NULL params in sss_parse_name
> >
> > src/util/usertools.c | 50 +++++++++++++++++++++++++++-----------------------
> > src/util/util.h      |  2 +-
> > 2 files changed, 28 insertions(+), 24 deletions(-)
>
> ACK
>
> LS

Pushed to master.

If there are any calls to sss_parse_name() that only pass in the
parameter to have a valid output pointer, can you remove those in a
follow up patch?

[Pavel Reichl]

[snip]
> If there are any calls to sss_parse_name() that only pass in the
> parameter to have a valid output pointer, can you remove those in a
> follow up patch?

Hi Jakub,

sure. Patch is attached.

Currently there is at least one such call in patch waiting for review. I
will fix it after the review is done.

PR

[Jakub Hrozek]

On Tue, Jan 28, 2014 at 05:32:04PM +0100, Pavel Reichl wrote:
> [snip]
> > If there are any calls to sss_parse_name() that only pass in the
> > parameter to have a valid output pointer, can you remove those in a
> > follow up patch?
>
> Hi Jakub,
>
> sure. Patch is attached.
>
> Currently there is at least one such call in patch waiting for review. I
> will fix it after the review is done.

The attached patch looks good to me and builds fine. So ACK to the
approach, let's push the patch after the subdomain_homedir one is
accepted.

[Jakub Hrozek]

On Tue, Jan 28, 2014 at 10:10:25PM +0100, Jakub Hrozek wrote:
> On Tue, Jan 28, 2014 at 05:32:04PM +0100, Pavel Reichl wrote:
> > [snip]
> > > If there are any calls to sss_parse_name() that only pass in the
> > > parameter to have a valid output pointer, can you remove those in a
> > > follow up patch?
> >
> > Hi Jakub,
> >
> > sure. Patch is attached.
> >
> > Currently there is at least one such call in patch waiting for review. I
> > will fix it after the review is done.
>
> The attached patch looks good to me and builds fine. So ACK to the
> approach, let's push the patch after the subdomain_homedir one is
> accepted.

I think we never pushed the patch even though it was acked.

Pushed to master now.