ehlo,
yet another warning from clang static analyser.
sss_krb5_princ_realm set output parameter realm to NULL and len to 0 in case of failure. Clang static analysers repoted warning "Null pointer passed as an argument to a 'nonnull' parameter" in function match_principal. It was possible, that realm_name with value NULL could be used in strncmp.
Function sss_krb5_princ_realm is used on other places for printing(formatting) realm_name and NULL can be safely used as a argument for printf-like functions.
Patch is attached.
LS
On 10/14/2013 11:50 AM, Lukas Slebodnik wrote:
ehlo,
yet another warning from clang static analyser.
sss_krb5_princ_realm set output parameter realm to NULL and len to 0 in case of failure. Clang static analysers repoted warning "Null pointer passed as an argument to a 'nonnull' parameter" in function match_principal. It was possible, that realm_name with value NULL could be used in strncmp.
Function sss_krb5_princ_realm is used on other places for printing(formatting) realm_name and NULL can be safely used as a argument for printf-like functions.
Patch is attached.
LS
The returned value should be checked on other references as well.
On Tue, Oct 15, 2013 at 01:59:28PM +0200, Pavel Březina wrote:
On 10/14/2013 11:50 AM, Lukas Slebodnik wrote:
ehlo,
yet another warning from clang static analyser.
sss_krb5_princ_realm set output parameter realm to NULL and len to 0 in case of failure. Clang static analysers repoted warning "Null pointer passed as an argument to a 'nonnull' parameter" in function match_principal. It was possible, that realm_name with value NULL could be used in strncmp.
Function sss_krb5_princ_realm is used on other places for printing(formatting) realm_name and NULL can be safely used as a argument for printf-like functions.
Patch is attached.
LS
The returned value should be checked on other references as well.
Hi Lukas,
are you planning to update this patch? I think it would be a good idea.
Additionally I would like to ask you if you can fix the HAVE_KRB5_PRINCIPAL_GET_REALM branch of sss_krb5_princ_realm(). Since krb5_principal_get_realm() may return NULL I think it is not a good idea to call strlen() unconditionally.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (13/03/14 13:11), Sumit Bose wrote:
On Tue, Oct 15, 2013 at 01:59:28PM +0200, Pavel Březina wrote:
On 10/14/2013 11:50 AM, Lukas Slebodnik wrote:
ehlo,
yet another warning from clang static analyser.
sss_krb5_princ_realm set output parameter realm to NULL and len to 0 in case of failure. Clang static analysers repoted warning "Null pointer passed as an argument to a 'nonnull' parameter" in function match_principal. It was possible, that realm_name with value NULL could be used in strncmp.
Function sss_krb5_princ_realm is used on other places for printing(formatting) realm_name and NULL can be safely used as a argument for printf-like functions.
Patch is attached.
LS
The returned value should be checked on other references as well.
Hi Lukas,
are you planning to update this patch? I think it would be a good idea.
Additionally I would like to ask you if you can fix the HAVE_KRB5_PRINCIPAL_GET_REALM branch of sss_krb5_princ_realm(). Since krb5_principal_get_realm() may return NULL I think it is not a good idea to call strlen() unconditionally.
bye, Sumit
This patch had very low priority in TODO list. I will try to send patch.
LS
On Thu, Mar 13, 2014 at 01:56:50PM +0100, Lukas Slebodnik wrote:
On (13/03/14 13:11), Sumit Bose wrote:
On Tue, Oct 15, 2013 at 01:59:28PM +0200, Pavel Březina wrote:
On 10/14/2013 11:50 AM, Lukas Slebodnik wrote:
ehlo,
yet another warning from clang static analyser.
sss_krb5_princ_realm set output parameter realm to NULL and len to 0 in case of failure. Clang static analysers repoted warning "Null pointer passed as an argument to a 'nonnull' parameter" in function match_principal. It was possible, that realm_name with value NULL could be used in strncmp.
Function sss_krb5_princ_realm is used on other places for printing(formatting) realm_name and NULL can be safely used as a argument for printf-like functions.
Patch is attached.
LS
The returned value should be checked on other references as well.
Hi Lukas,
are you planning to update this patch? I think it would be a good idea.
Additionally I would like to ask you if you can fix the HAVE_KRB5_PRINCIPAL_GET_REALM branch of sss_krb5_princ_realm(). Since krb5_principal_get_realm() may return NULL I think it is not a good idea to call strlen() unconditionally.
bye, Sumit
This patch had very low priority in TODO list. I will try to send patch.
np, I agree that this is low priority, I just wanted to check if it got lost.
bye, Sumit
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (13/03/14 14:54), Sumit Bose wrote:
On Thu, Mar 13, 2014 at 01:56:50PM +0100, Lukas Slebodnik wrote:
On (13/03/14 13:11), Sumit Bose wrote:
On Tue, Oct 15, 2013 at 01:59:28PM +0200, Pavel Březina wrote:
On 10/14/2013 11:50 AM, Lukas Slebodnik wrote:
ehlo,
yet another warning from clang static analyser.
sss_krb5_princ_realm set output parameter realm to NULL and len to 0 in case of failure. Clang static analysers repoted warning "Null pointer passed as an argument to a 'nonnull' parameter" in function match_principal. It was possible, that realm_name with value NULL could be used in strncmp.
Function sss_krb5_princ_realm is used on other places for printing(formatting) realm_name and NULL can be safely used as a argument for printf-like functions.
Patch is attached.
LS
The returned value should be checked on other references as well.
Hi Lukas,
are you planning to update this patch? I think it would be a good idea.
Additionally I would like to ask you if you can fix the HAVE_KRB5_PRINCIPAL_GET_REALM branch of sss_krb5_princ_realm(). Since krb5_principal_get_realm() may return NULL I think it is not a good idea to call strlen() unconditionally.
bye, Sumit
This patch had very low priority in TODO list. I will try to send patch.
np, I agree that this is low priority, I just wanted to check if it got lost.
bye, Sumit
No, beacuse it is our patchwork with status "change requested"
LS
On (13/03/14 13:11), Sumit Bose wrote:
On Tue, Oct 15, 2013 at 01:59:28PM +0200, Pavel Březina wrote:
On 10/14/2013 11:50 AM, Lukas Slebodnik wrote:
ehlo,
yet another warning from clang static analyser.
sss_krb5_princ_realm set output parameter realm to NULL and len to 0 in case of failure. Clang static analysers repoted warning "Null pointer passed as an argument to a 'nonnull' parameter" in function match_principal. It was possible, that realm_name with value NULL could be used in strncmp.
Function sss_krb5_princ_realm is used on other places for printing(formatting) realm_name and NULL can be safely used as a argument for printf-like functions.
Patch is attached.
LS
The returned value should be checked on other references as well.
Hi Lukas,
are you planning to update this patch? I think it would be a good idea.
Additionally I would like to ask you if you can fix the HAVE_KRB5_PRINCIPAL_GET_REALM branch of sss_krb5_princ_realm(). Since krb5_principal_get_realm() may return NULL I think it is not a good idea to call strlen() unconditionally.
It is 1st year aniversary of the patch. So it is the right time to send updated version. :-)
Two patches are attached.
LS
On 10/14/2014 01:59 PM, Lukas Slebodnik wrote:
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c index b4012593d96bc951143e4bb2ba7a91d118b1a53c..7daedb47464917966350f94c7d5b0844ccee7edb 100644 --- a/src/util/sss_krb5.c +++ b/src/util/sss_krb5.c @@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts, void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ, const char **realm, int *len) {
- *realm = krb5_principal_get_realm(context, princ);
- *len = strlen(*realm);
- const char *realm_str = krb5_principal_get_realm(context, princ);
- if (realm_str != NULL) {
*realm = krb5_principal_get_realm(context, princ);
Why do we have to call krb5_principal_get_realm() again here? Why can't we just reuse value in realm_str?
*len = strlen(*realm);
- } else {
*realm = NULL;
*len = 0;
- } }
Do we need realm_str at all? Can't we handle the check just with realm?
#else void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ, --
Thanks for answering!
On (14/10/14 14:14), Pavel Reichl wrote:
On 10/14/2014 01:59 PM, Lukas Slebodnik wrote:
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c index b4012593d96bc951143e4bb2ba7a91d118b1a53c..7daedb47464917966350f94c7d5b0844ccee7edb 100644 --- a/src/util/sss_krb5.c +++ b/src/util/sss_krb5.c @@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts, void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ, const char **realm, int *len) {
- *realm = krb5_principal_get_realm(context, princ);
- *len = strlen(*realm);
- const char *realm_str = krb5_principal_get_realm(context, princ);
- if (realm_str != NULL) {
*realm = krb5_principal_get_realm(context, princ);
Why do we have to call krb5_principal_get_realm() again here? Why can't we just reuse value in realm_str?
I just forgot to squash changes before sending patches.
Updated version is attached.
LS
On (14/10/14 14:44), Lukas Slebodnik wrote:
On (14/10/14 14:14), Pavel Reichl wrote:
On 10/14/2014 01:59 PM, Lukas Slebodnik wrote:
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c index b4012593d96bc951143e4bb2ba7a91d118b1a53c..7daedb47464917966350f94c7d5b0844ccee7edb 100644 --- a/src/util/sss_krb5.c +++ b/src/util/sss_krb5.c @@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts, void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ, const char **realm, int *len) {
- *realm = krb5_principal_get_realm(context, princ);
- *len = strlen(*realm);
- const char *realm_str = krb5_principal_get_realm(context, princ);
- if (realm_str != NULL) {
*realm = krb5_principal_get_realm(context, princ);
Why do we have to call krb5_principal_get_realm() again here? Why can't we just reuse value in realm_str?
I just forgot to squash changes before sending patches.
Updated version is attached.
Attached are updated patches which can be applied on current master.
LS
On 11/25/2014 03:33 PM, Lukas Slebodnik wrote:
On (14/10/14 14:44), Lukas Slebodnik wrote:
On (14/10/14 14:14), Pavel Reichl wrote:
On 10/14/2014 01:59 PM, Lukas Slebodnik wrote:
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c index b4012593d96bc951143e4bb2ba7a91d118b1a53c..7daedb47464917966350f94c7d5b0844ccee7edb 100644 --- a/src/util/sss_krb5.c +++ b/src/util/sss_krb5.c @@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts, void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ, const char **realm, int *len) {
- *realm = krb5_principal_get_realm(context, princ);
- *len = strlen(*realm);
- const char *realm_str = krb5_principal_get_realm(context, princ);
- if (realm_str != NULL) {
*realm = krb5_principal_get_realm(context, princ);
Why do we have to call krb5_principal_get_realm() again here? Why can't we just reuse value in realm_str?
I just forgot to squash changes before sending patches.
Updated version is attached.
Attached are updated patches which can be applied on current master.
LS
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I think you rebased wrong patch - there were some changes which are not in this patch - you can see my prev post and you reaction to see what I mean.
On (25/11/14 16:32), Pavel Reichl wrote:
On 11/25/2014 03:33 PM, Lukas Slebodnik wrote:
On (14/10/14 14:44), Lukas Slebodnik wrote:
On (14/10/14 14:14), Pavel Reichl wrote:
On 10/14/2014 01:59 PM, Lukas Slebodnik wrote:
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c index b4012593d96bc951143e4bb2ba7a91d118b1a53c..7daedb47464917966350f94c7d5b0844ccee7edb 100644 --- a/src/util/sss_krb5.c +++ b/src/util/sss_krb5.c @@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts, void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ, const char **realm, int *len) {
- *realm = krb5_principal_get_realm(context, princ);
- *len = strlen(*realm);
- const char *realm_str = krb5_principal_get_realm(context, princ);
- if (realm_str != NULL) {
*realm = krb5_principal_get_realm(context, princ);
Why do we have to call krb5_principal_get_realm() again here? Why can't we just reuse value in realm_str?
I just forgot to squash changes before sending patches.
Updated version is attached.
Attached are updated patches which can be applied on current master.
LS
I think you rebased wrong patch - there were some changes which are not in this patch - you can see my prev post and you reaction to see what I mean.
yep,
I just sent wrong patch. The problematic was the 2nd patch.
LS
On 11/25/2014 04:48 PM, Lukas Slebodnik wrote:
On (25/11/14 16:32), Pavel Reichl wrote:
On 11/25/2014 03:33 PM, Lukas Slebodnik wrote:
On (14/10/14 14:44), Lukas Slebodnik wrote:
On (14/10/14 14:14), Pavel Reichl wrote:
On 10/14/2014 01:59 PM, Lukas Slebodnik wrote:
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c index b4012593d96bc951143e4bb2ba7a91d118b1a53c..7daedb47464917966350f94c7d5b0844ccee7edb 100644 --- a/src/util/sss_krb5.c +++ b/src/util/sss_krb5.c @@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts, void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ, const char **realm, int *len) {
- *realm = krb5_principal_get_realm(context, princ);
- *len = strlen(*realm);
- const char *realm_str = krb5_principal_get_realm(context, princ);
- if (realm_str != NULL) {
*realm = krb5_principal_get_realm(context, princ);
Why do we have to call krb5_principal_get_realm() again here? Why can't we just reuse value in realm_str?
I just forgot to squash changes before sending patches.
Updated version is attached.
Attached are updated patches which can be applied on current master.
LS
I think you rebased wrong patch - there were some changes which are not in this patch - you can see my prev post and you reaction to see what I mean.
yep,
I just sent wrong patch. The problematic was the 2nd patch.
LS
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
LGTM, builds fine. ACK
On Tue, Nov 25, 2014 at 05:18:47PM +0100, Pavel Reichl wrote:
I just sent wrong patch. The problematic was the 2nd patch.
LS
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
LGTM, builds fine. ACK
master: 2dc519ba98ca886076ba9e16b95a72732909cea1 466f5a539be1e4c6e7cfb396a2f406e1eb8c428d
sssd-devel@lists.fedorahosted.org