On Sat, 2012-02-18 at 12:55 +0100, Jakub Hrozek wrote:
On Sat, Feb 18, 2012 at 10:15:47AM +0100, Jakub Hrozek wrote:
> Provides a static storage that keeps track of getgr[nam|gid] results
in
> case the buffer supplied by caller is not big enough.
>
>
https://fedorahosted.org/sssd/ticket/1191
>
Self-nack, the original patch was leaking memory. A new patch is
attached.
NACK,
see inline
plain text
document
attachment
(0001-Save-getgr-results-if-the-glibc-buffer-wouldn-t-fit.patch)
From cba6b15e92227312f72a267fd5875f57c4ae9065 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Wed, 15 Feb 2012 17:07:56 +0100
Subject: [PATCH] Save getgr results if the glibc buffer wouldn't fit
Provides a static storage that keeps track of getgr[nam|gid] results
in
case the buffer supplied by caller is not big enough.
https://fedorahosted.org/sssd/ticket/1191
---
src/sss_client/nss_group.c | 181
+++++++++++++++++++++++++++++++++++++++++---
1 files changed, 170 insertions(+), 11 deletions(-)
diff --git a/src/sss_client/nss_group.c b/src/sss_client/nss_group.c
index
7e5f79ad5d17d1065fdbd647ed97331e37e18d80..891dfcac430fb9dd1c008544ec2b70d822172f4e
100644
--- a/src/sss_client/nss_group.c
+++ b/src/sss_client/nss_group.c
@@ -27,15 +27,23 @@
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
+#include <stdbool.h>
#include "sss_cli.h"
+#define FILL_GRREP(grrep, result, buffer, buflen) { \
+ grrep.result = result; \
+ grrep.buffer = buffer; \
+ grrep.buflen = buflen; \
+} while(0);
+
static struct sss_nss_getgrent_data {
size_t len;
size_t ptr;
uint8_t *data;
} sss_nss_getgrent_data;
-static void sss_nss_getgrent_data_clean(void) {
+static void sss_nss_getgrent_data_clean(void)
+{
if (sss_nss_getgrent_data.data != NULL) {
free(sss_nss_getgrent_data.data);
@@ -45,6 +53,110 @@ static void sss_nss_getgrent_data_clean(void) {
sss_nss_getgrent_data.ptr = 0;
}
+static struct sss_nss_getgr_data {
+ union {
+ char *grname;
+ gid_t gid;
+ } id;
+ uint8_t *repbuf;
+ size_t replen;
+} sss_nss_getgr_data;
You should add a type here, as is this structure is unsafe.
Add:
enum data_type = {
GETGR_NONE,
GETGR_NAME,
GETGR_GID
} type;
+static void sss_nss_getgr_data_clean(void)
+{
+ if (sss_nss_getgr_data.repbuf) {
+ free(sss_nss_getgr_data.repbuf);
+ sss_nss_getgr_data.repbuf = NULL;
+ }
+ sss_nss_getgr_data.replen = 0;
+}
+
+static void sss_nss_getgrnam_data_clean(void)
+{
+ if (sss_nss_getgr_data.id.grname) {
+ free(sss_nss_getgr_data.id.grname);
+ sss_nss_getgr_data.id.grname = NULL;
+ }
+ sss_nss_getgr_data_clean();
+}
+
+static void sss_nss_getgrgid_data_clean(void)
+{
+ sss_nss_getgr_data.id.grname = 0;
+ sss_nss_getgr_data_clean();
+}
Please collapse the above 3 functions into a single one that act based
on the type defined above.
+static void sss_nss_getgr_get(uint8_t **repbuf, size_t *replen)
+{
+ *repbuf = sss_nss_getgr_data.repbuf;
+ *replen = sss_nss_getgr_data.replen;
+}
I am honestly not sure it is worth to have a getter, when all you do is
assign 2 buffers (same comment for the setter.
+static errno_t sss_nss_getgrnam_again(const char *name, uint8_t
**repbuf,
+ size_t *replen)
+{
+ if (sss_nss_getgr_data.id.grname &&
+ strcmp(sss_nss_getgr_data.id.grname, name) == 0) {
This is wrong, you do not know what was stored before it cuold have been
a gid. Without storing and then checking the type, you are going to test
a gid value as it was a memory pointer. In most cases this will be a
segfault in the application!!
+ sss_nss_getgr_get(repbuf, replen);
+ return EOK;
+ }
+
+ return ENOENT;
+}
+
+static errno_t sss_nss_getgrgid_again(gid_t gid, uint8_t **repbuf,
+ size_t *replen)
+{
+ if (gid && sss_nss_getgr_data.id.gid == gid) {
This won't cause a segfault, but checking a gid against potentially a
pointer could result in a match in a very unfortunate case, returning
bad info.
+ sss_nss_getgr_get(repbuf, replen);
+ return EOK;
+ }
+
+ return ENOENT;
+}
+
+static void sss_nss_getgr_save(uint8_t *repbuf, size_t replen)
+{
+ sss_nss_getgr_data.repbuf = repbuf;
+ sss_nss_getgr_data.replen = replen;
+}
See above, setter for 2 assignments seem a bit too much.
+static errno_t sss_nss_getgrnam_save(const char *name, const int
namelen,
+ uint8_t *repbuf, size_t replen)
+{
+ if (sss_nss_getgr_data.id.grname) {
+ if (strcmp(sss_nss_getgr_data.id.grname, name) == 0) {
Segfault case again.
+ /* We already keep track of this request */
+ return EOK;
+ }
+
+ /* This is a new request, make sure there are no leftovers
+ * from last time
+ */
+ sss_nss_getgrnam_data_clean();
+ }
+
+ sss_nss_getgr_data.id.grname = strndup(name, namelen);
+ if (!sss_nss_getgr_data.id.grname) return ENOMEM;
+
+ sss_nss_getgr_save(repbuf, replen);
+ return EOK;
+}
+
+static errno_t sss_nss_getgrgid_save(const gid_t gid, uint8_t
*repbuf,
+ size_t replen)
+{
+ if (sss_nss_getgr_data.id.gid &&
+ gid == sss_nss_getgr_data.id.gid) {
Again comparison might give fault result as above.
+ sss_nss_getgrgid_data_clean();
+ return EOK;
+ }
+
+ sss_nss_getgr_data.id.gid = gid;
+ sss_nss_getgr_save(repbuf, replen);
+ return EOK;
+}
+
/* GETGRNAM Request:
*
* 0-X: string with name
@@ -231,6 +343,7 @@ enum nss_status _nss_sss_getgrnam_r(const char
*name, struct group *result,
size_t replen, len, name_len;
enum nss_status nret;
int ret;
+ bool fast;
/* Caught once glibc passing in buffer == 0x0 */
if (!buffer || !buflen) {
@@ -244,20 +357,27 @@ enum nss_status _nss_sss_getgrnam_r(const char
*name, struct group *result,
return NSS_STATUS_NOTFOUND;
}
+ sss_nss_lock();
+
+ fast = false;
+ ret = sss_nss_getgrnam_again(name, &repbuf, &replen);
+ if (ret == EOK) {
+ fast = true;
+ FILL_GRREP(grrep, result, buffer, buflen);
+ goto reply;
This got is a bad idea, makes reviewing this code diffcult, and you are
not chasing an error condition.
Use gotos only as exceptions, any other use is not acceptable.
+ }
+ sss_nss_getgrnam_data_clean();
+
rd.len = name_len + 1;
rd.data = name;
- sss_nss_lock();
-
nret = sss_nss_make_request(SSS_NSS_GETGRNAM, &rd,
&repbuf, &replen, errnop);
if (nret != NSS_STATUS_SUCCESS) {
goto out;
}
- grrep.result = result;
- grrep.buffer = buffer;
- grrep.buflen = buflen;
+ FILL_GRREP(grrep, result, buffer, buflen);
/* no results if not found */
if (((uint32_t *)repbuf)[0] == 0) {
@@ -274,9 +394,24 @@ enum nss_status _nss_sss_getgrnam_r(const char
*name, struct group *result,
goto out;
}
+reply:
len = replen - 8;
ret = sss_nss_getgr_readrep(&grrep, repbuf+8, &len);
- free(repbuf);
+ if (ret == ERANGE) {
+ /* The buffer glibc supplied us with was too small. Let's keep
+ * the results in memory if glibc retries with a bigger buffer
*/
+ if (sss_nss_getgrnam_save(name, name_len, repbuf, replen) !=
EOK) {
+ /* Not fatal, we'll just go to SSSD next time */
+ free(repbuf);
+ }
+ } else {
+ if (fast) {
+ sss_nss_getgrnam_data_clean();
+ } else {
+ free(repbuf);
+ }
+ }
Bundle the above up into a function, remove fast and handle cleanup
where you now goto. Remember to unlock.
if (ret) {
*errnop = ret;
nret = NSS_STATUS_TRYAGAIN;
@@ -300,6 +435,7 @@ enum nss_status _nss_sss_getgrgid_r(gid_t gid,
struct group *result,
enum nss_status nret;
uint32_t group_gid;
int ret;
+ bool fast;
/* Caught once glibc passing in buffer == 0x0 */
if (!buffer || !buflen) return ERANGE;
@@ -310,15 +446,22 @@ enum nss_status _nss_sss_getgrgid_r(gid_t gid,
struct group *result,
sss_nss_lock();
+ fast = false;
+ ret = sss_nss_getgrgid_again(gid, &repbuf, &replen);
+ if (ret == EOK) {
+ fast = true;
+ FILL_GRREP(grrep, result, buffer, buflen);
+ goto reply;
+ }
+ sss_nss_getgrgid_data_clean();
+
nret = sss_nss_make_request(SSS_NSS_GETGRGID, &rd,
&repbuf, &replen, errnop);
if (nret != NSS_STATUS_SUCCESS) {
goto out;
}
- grrep.result = result;
- grrep.buffer = buffer;
- grrep.buflen = buflen;
+ FILL_GRREP(grrep, result, buffer, buflen);
/* no results if not found */
if (((uint32_t *)repbuf)[0] == 0) {
@@ -335,9 +478,25 @@ enum nss_status _nss_sss_getgrgid_r(gid_t gid,
struct group *result,
goto out;
}
+reply:
len = replen - 8;
ret = sss_nss_getgr_readrep(&grrep, repbuf+8, &len);
- free(repbuf);
+ if (ret == ERANGE) {
+ /* The buffer glibc supplied us with was too small. Let's keep
+ * the results in memory if glibc retries with a bigger buffer
*/
+ if (sss_nss_getgrgid_save(gid, repbuf, replen) != EOK) {
+ /* Not fatal, we'll just go to SSSD next time */
+ free(repbuf);
+ }
+ } else {
+ if (fast) {
+ sss_nss_getgrgid_data_clean();
+ } else {
+ free(repbuf);
+ }
+ }
+
+
Same as above.
if (ret) {
*errnop = ret;
nret = NSS_STATUS_TRYAGAIN;
--
1.7.7.6
Simo.
--
Simo Sorce * Red Hat, Inc * New York