SSSD / sssd

Clone

#3589 nss: Fix invalid enum nss_status return values.
Closed 6 years ago by lslebodn. Opened 6 years ago by codonell.
Unknown source master  into  master

file modified
+8 -2 
@@ -522,7 +522,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      ret = sss_nss_mc_getgrgid(gid, result, buffer, buflen);
 

      switch (ret) {
 
@@ -655,7 +658,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      /* if there are leftovers return the next one */
 

      if (sss_nss_getgrent_data.data != NULL &&

file modified
+4 -1 
@@ -231,7 +231,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      /* If we're already processing result data, continue to
 

       * return it.

file modified
+8 -2 
@@ -251,7 +251,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      ret = sss_nss_mc_getpwuid(uid, result, buffer, buflen);
 

      switch (ret) {
 
@@ -376,7 +379,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      /* if there are leftovers return the next one */
 

      if (sss_nss_getpwent_data.data != NULL &&

file modified
+12 -3 
@@ -177,7 +177,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      ret = sss_strnlen(name, SSS_NAME_MAX, &name_len);
 

      if (ret != 0) {
 
@@ -278,7 +281,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      if (protocol) {
 

          ret = sss_strnlen(protocol, SSS_NAME_MAX, &proto_len);
 
@@ -411,7 +417,10 @@
 

      int ret;
 

  

      /* Caught once glibc passing in buffer == 0x0 */
 

-     if (!buffer || !buflen) return ERANGE;
 

+     if (!buffer || !buflen) {
 

+ 	*errnop = ERANGE;
 

+ 	return NSS_STATUS_TRYAGAIN;
 

+     }
 

  

      /* if there are leftovers return the next one */
 

      if (sss_nss_getservent_data.data != NULL &&

The upstream glibc test nss/bug17079 covers several cases where the
NSS infrastructure passes invalid pointers to NSS plugins. The plugins
should return correct results for the invalid values e.g. ERANGE,
but it should do so by setting errnop to the error and returning
NSS_STATUS_TRYAGAIN. This commit fixes the group, netgroup, passwd
and service handling code to correctly return ERANGE in errnop
and NSS_TATUS_TRYAGAIN in the case of invalid buffer (NULL) or
zero sized buffer length. This fixes the nss/bug17079 regression test
when run in a test configuration with sss enabled for any of the
above mentioned services.

Upstream glibc bug:
Bug 22530 - FAIL: nss/bug17079 due to _nss_sss_getpwuid_r
https://sourceware.org/bugzilla/show_bug.cgi?id=22530

Signed-off-by: Carlos O'Donell carlos@redhat.com

This doesn't look right:

 5 -     if (!buffer || !buflen) return ERANGE;
 6 +     if (!buffer || !buflen) {
 7 + »       *errnop = ERANGE;
 8 + »       return NSS_STATUS_SUCCESS;
 9 +     }

It should be NSS_STATUS_TRYAGAIN.

Thank you guys for patch.

Could you also check other parts in the same file.
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/common.c#_734
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/common.c#_749
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/common.c#_769

NSS_STATUS_NOTFOUND is returned only with enabled switch --enable-sss-default-nss-plugin. It is enabled in fedora and el7 but disabled by default in upstream. Is it still necessary to have it disabled by default in upstream? Can we get rid of conditional build?

Related tickets:
https://pagure.io/SSSD/sssd/issue/2439
https://bugzilla.redhat.com/show_bug.cgi?id=988068#c13

Uhm so why is this NSS_STATUS_SUCCESS ?

This doesn't look right:
5 - if (!buffer || !buflen) return ERANGE;
6 + if (!buffer || !buflen) {
7 + » *errnop = ERANGE;
8 + » return NSS_STATUS_SUCCESS;
9 + }

Correct, this was a mistake on my part, just a mental typo. Thanks for catching this.

Could you also check other parts in the same file.
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/common.c#_734
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/common.c#_749
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/common.c#_769
NSS_STATUS_NOTFOUND is returned only with enabled switch --enable-sss-default-nss-plugin. It is enabled in fedora and el7 but disabled by default in upstream. Is it still necessary to have it disabled by default in upstream? Can we get rid of conditional build?

Related tickets:
https://pagure.io/SSSD/sssd/issue/2439
https://bugzilla.redhat.com/show_bug.cgi?id=988068#c13

This is a distinct issue and should be discussed in another forum. Please feel free to email me aobut this issue or file a new PR and CC me.

1 new commit added

  • nss: Fix invalid enum nss_status return values.
6 years ago

Updated my fork with the fixed NSS status code. Should be good now.

Tested local build OK for password service.

Tested x86_64 glibc + sssd only.

Tested x86_64 glibc + sssd only.

@codonell I ran some downstream tests and they passed as well.

Would you mind if I squash patches to a single patch before merging?

And I assume that this patch is not related to any version of glibc and therefore can be pushed also to older branches. Is it correct assumption?

Tested x86_64 glibc + sssd only.

@codonell I ran some downstream tests and they passed as well.
Would you mind if I squash patches to a single patch before merging?
And I assume that this patch is not related to any version of glibc and therefore can be pushed also to older branches. Is it correct assumption?

Yes, please squash the patches into a single commit.

The patch is OK to apply everywhere. It has no dependency on glibc versions.

The fix will help our glibc testing configurations that have sssd.

master:

sssd-1-14:

sssd-1-13:

Pull-Request has been closed by lslebodn
6 years ago

I'll do new update of sssd in fedora soon so I will backport this patch as well.

Thank you very much guys.

I'll do new update of sssd in fedora soon so I will backport this patch as well.
Thank you very much guys.

Thank you for helping us get the fix in place!

@codonell fedora will soon have fixed version in updates-testing. packages were pushed yesterday for f25+
Metadata
None
No Tags
Changes Summary 4
+8 -2
file changed
src/sss_client/nss_group.c
+4 -1
file changed
src/sss_client/nss_netgroup.c
+8 -2
file changed
src/sss_client/nss_passwd.c
+12 -3
file changed
src/sss_client/nss_services.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.