New patch with code review concerns addressed.
On 09/10/2009 04:37 PM, Simo Sorce wrote:
> On Thu, 2009-09-10 at 16:02 -0400, Stephen Gallagher wrote:
>> New approach attached.
>>
>> Patch 0001: Create two convenience functions, strtouint32 and
>> strtoint32. These behave identically to strtol or strtoll, except that
>> they are guaranteed to be constrained to 32 bits.
>
> I don't like the ifdefs, we should always use long long and bail out in
> configure if it is not a 64bit quantity.
>
> Other comments inline.
>
>> Patch 0002: Read the minId and maxId values as a string, testing them
>> for negation and zero-length before passing them to the function
>> created
>> in patch 0001 to convert it to a 32-bit value.
>
> Ack this one.
>
>
>
>>
>>
>>
>>
>>
>> plain text
>> document
>> attachment
>> (0001-Add-strtoint32-and-strtouint32-convenience-functions.patch)
>
> [..]
>
>> diff --git a/server/util/strtonum.c b/server/util/strtonum.c
>> new file mode 100644
>> index 0000000..f7fa5a5
>> --- /dev/null
>> +++ b/server/util/strtonum.c
>> @@ -0,0 +1,120 @@
>> +/*
>> + SSSD
>> +
>> + InfoPipe
>
> errr ^^^^^^^^ ?
>
>> + Copyright (C) Stephen Gallagher <sgall...@redhat.com> 2009
>
> [..]
>
>> +/* strtoint32 */
>> +uint32_t strtoint32(const char *nptr, char **endptr, int base)
>
> ^^^^^^^^ I guess this should be int32_t not unit32_t ...
>
>> +{
>> +#if SIZEOF_LONG>=4
>> + long ret = 0;
>> + errno = 0;
>> + ret = strtoul(nptr, endptr, base);
>
> and this should be strtol(), not strtoul() ... these defines are bad for
> this kind of errors exactly, please avoid it and always use long long.
>
> [..]
>
>> diff --git a/server/util/strtonum.h b/server/util/strtonum.h
>> new file mode 100644
>> index 0000000..ba4cf16
>> --- /dev/null
>> +++ b/server/util/strtonum.h
>> @@ -0,0 +1,32 @@
>
> [..]
>
>> +#ifndef _STRTONUM_H_
>> +#define _STRTONUM_H_
>> +
>> +#include <ctype.h>
>> +#include <stdlib.h>
>> +#include <stdint.h>
>> +
>> +uint32_t strtoint32(const char *nptr, char **endptr, int base);
>
> ^^^^^^^^ again int32_t not uint32_t
>
>> +uint32_t strtouint32(const char *nptr, char **endptr, int base);
>> +
>> +#endif /* _STRTONUM_H_ */
>
>
> The rest looks ok.
>
> Simo.
> -- Stephen Gallagher RHCE 804006346421761 Looking to carve out IT costs? www.redhat.com/carveoutcosts/
From 018a3b69840f01d5e94dedcd8f680253bcc0f546 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Thu, 10 Sep 2009 15:56:52 -0400
Subject: [PATCH 1/4] Add strtoint32 and strtouint32 convenience functions
---
server/Makefile.am | 2 +
server/configure.ac | 1 +
server/external/sizes.m4 | 44 ++++++++++++++++++++++++++
server/util/strtonum.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
server/util/strtonum.h | 32 +++++++++++++++++++
5 files changed, 156 insertions(+), 0 deletions(-)
create mode 100644 server/external/sizes.m4
create mode 100644 server/util/strtonum.c
create mode 100644 server/util/strtonum.h
diff --git a/server/Makefile.am b/server/Makefile.am
index 27ac01d..18fb699 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -160,6 +160,7 @@ SSSD_UTIL_OBJ = \
util/signal.c \
util/usertools.c \
util/backup_file.c \
+ util/strtonum.c \
$(SSSD_DEBUG_OBJ)
SSSD_RESPONDER_OBJ = \
@@ -203,6 +204,7 @@ dist_noinst_HEADERS = \
util/dlinklist.h \
util/sssd-i18n.h \
util/util.h \
+ util/strtonum.h \
config.h \
monitor/monitor.h \
monitor/monitor_interfaces.h \
diff --git a/server/configure.ac b/server/configure.ac
index 8e7ea0b..3320507 100644
--- a/server/configure.ac
+++ b/server/configure.ac
@@ -61,6 +61,7 @@ m4_include([external/libpcre.m4])
m4_include([external/krb5.m4])
m4_include([external/libcares.m4])
m4_include([external/docbook.m4])
+m4_include([external/sizes.m4])
m4_include([util/signal.m4])
PKG_CHECK_MODULES([DBUS],[dbus-1])
diff --git a/server/external/sizes.m4 b/server/external/sizes.m4
new file mode 100644
index 0000000..53df61d
--- /dev/null
+++ b/server/external/sizes.m4
@@ -0,0 +1,44 @@
+# Solaris needs HAVE_LONG_LONG defined
+AC_CHECK_TYPES(long long)
+
+AC_CHECK_SIZEOF(int)
+AC_CHECK_SIZEOF(char)
+AC_CHECK_SIZEOF(short)
+AC_CHECK_SIZEOF(long)
+AC_CHECK_SIZEOF(long long)
+
+if test $ac_cv_sizeof_long_long -lt 8 ; then
+AC_MSG_ERROR([SSSD requires long long of 64-bits])
+fi
+
+AC_CHECK_TYPE(uint_t, unsigned int)
+AC_CHECK_TYPE(int8_t, char)
+AC_CHECK_TYPE(uint8_t, unsigned char)
+AC_CHECK_TYPE(int16_t, short)
+AC_CHECK_TYPE(uint16_t, unsigned short)
+
+if test $ac_cv_sizeof_int -eq 4 ; then
+AC_CHECK_TYPE(int32_t, int)
+AC_CHECK_TYPE(uint32_t, unsigned int)
+elif test $ac_cv_size_long -eq 4 ; then
+AC_CHECK_TYPE(int32_t, long)
+AC_CHECK_TYPE(uint32_t, unsigned long)
+else
+AC_MSG_ERROR([LIBREPLACE no 32-bit type found])
+fi
+
+AC_CHECK_TYPE(int64_t, long long)
+AC_CHECK_TYPE(uint64_t, unsigned long long)
+
+AC_CHECK_TYPE(size_t, unsigned int)
+AC_CHECK_TYPE(ssize_t, int)
+
+AC_CHECK_SIZEOF(off_t)
+AC_CHECK_SIZEOF(size_t)
+AC_CHECK_SIZEOF(ssize_t)
+
+AC_CHECK_TYPE(intptr_t, long long)
+AC_CHECK_TYPE(uintptr_t, unsigned long long)
+AC_CHECK_TYPE(ptrdiff_t, unsigned long long)
+
+
diff --git a/server/util/strtonum.c b/server/util/strtonum.c
new file mode 100644
index 0000000..69673c6
--- /dev/null
+++ b/server/util/strtonum.c
@@ -0,0 +1,77 @@
+/*
+ SSSD
+
+ SSSD Utility functions
+
+ Copyright (C) Stephen Gallagher <sgall...@redhat.com> 2009
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <ctype.h>
+#include <stdlib.h>
+#include <errno.h>
+#include "config.h"
+#include "util/util.h"
+#include "util/strtonum.h"
+
+/* strtoint32 */
+int32_t strtoint32(const char *nptr, char **endptr, int base)
+{
+ long long ret = 0;
+ errno = 0;
+ ret = strtoll(nptr, endptr, base);
+ if (ret == LLONG_MIN) {
+ if (errno == ERANGE) {
+ return INT32_MIN;
+ }
+ }
+ else if (ret == LLONG_MAX) {
+ if (errno == ERANGE) {
+ return INT32_MAX;
+ }
+ }
+ else if (ret > INT32_MAX) {
+ errno = ERANGE;
+ return INT32_MAX;
+ }
+ else if (ret < INT32_MIN) {
+ errno = ERANGE;
+ return INT32_MIN;
+ }
+
+ errno = 0;
+ return (int32_t)ret;
+}
+
+
+/* strtouint32 */
+uint32_t strtouint32(const char *nptr, char **endptr, int base)
+{
+ long long ret = 0;
+ errno = 0;
+ ret = strtoull(nptr, endptr, base);
+ if (ret == ULLONG_MAX) {
+ if (errno == ERANGE) {
+ return UINT32_MAX;
+ }
+ }
+ else if (ret > UINT32_MAX) {
+ errno = ERANGE;
+ return UINT32_MAX;
+ }
+
+ errno = 0;
+ return (uint32_t)ret;
+}
diff --git a/server/util/strtonum.h b/server/util/strtonum.h
new file mode 100644
index 0000000..4509596
--- /dev/null
+++ b/server/util/strtonum.h
@@ -0,0 +1,32 @@
+/*
+ SSSD
+
+ SSSD Utility functions
+
+ Copyright (C) Stephen Gallagher 2009
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef _STRTONUM_H_
+#define _STRTONUM_H_
+
+#include <ctype.h>
+#include <stdlib.h>
+#include <stdint.h>
+
+int32_t strtoint32(const char *nptr, char **endptr, int base);
+uint32_t strtouint32(const char *nptr, char **endptr, int base);
+
+#endif /* _STRTONUM_H_ */
--
1.6.2.5
From fb1aed04d99f7f4b6a932595770d6cb72bac4ed5 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Thu, 10 Sep 2009 11:07:25 -0400
Subject: [PATCH 2/4] Properly detect negative/invalid values for the minId and
maxId
---
server/confdb/confdb.c | 61 +++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/server/confdb/confdb.c b/server/confdb/confdb.c
index 778345f..27daee8 100644
--- a/server/confdb/confdb.c
+++ b/server/confdb/confdb.c
@@ -21,11 +21,13 @@
#define _GNU_SOURCE
+#include <ctype.h>
#include "config.h"
#include "util/util.h"
#include "confdb/confdb.h"
#include "confdb/confdb_private.h"
#include "util/btreemap.h"
+#include "util/strtonum.h"
#include "db/sysdb.h"
#define CONFDB_DOMAINS_PATH "config/domains"
@@ -673,6 +675,41 @@ int confdb_init(TALLOC_CTX *mem_ctx,
return EOK;
}
+static errno_t get_entry_as_uint32(struct ldb_message *msg,
+ uint32_t *return_value,
+ const char *entry,
+ uint32_t default_value)
+{
+ const char *tmp = NULL;
+ char *endptr;
+ uint32_t u32ret = 0;
+
+ tmp = ldb_msg_find_attr_as_string(msg, entry, NULL);
+ if (tmp == NULL) {
+ *return_value = default_value;
+ return EOK;
+ }
+
+ if ((*tmp == '-') || (*tmp == '\0')) {
+ return EINVAL;
+ }
+
+ errno = 0;
+ u32ret = strtouint32 (tmp, &endptr, 10);
+ if (u32ret == UINT32_MAX) {
+ if (errno) {
+ return errno;
+ }
+ }
+ if (*endptr != '\0') {
+ /* Not all of the string was a valid number */
+ return EINVAL;
+ }
+
+ *return_value = u32ret;
+ return EOK;
+}
+
static int confdb_get_domain_internal(struct confdb_ctx *cdb,
TALLOC_CTX *mem_ctx,
const char *name,
@@ -780,12 +817,24 @@ static int confdb_get_domain_internal(struct confdb_ctx
*cdb,
domain->fqnames = true;
}
- domain->id_min = ldb_msg_find_attr_as_uint(res->msgs[0],
- "minId", SSSD_MIN_ID);
- domain->id_max = ldb_msg_find_attr_as_uint(res->msgs[0],
- "maxId", 0);
- if ((domain->id_max && (domain->id_max < domain->id_min)) ||
- (domain->id_min < 0)){
+ ret = get_entry_as_uint32(res->msgs[0], &domain->id_min,
+ "minId", SSSD_MIN_ID);
+ if (ret != EOK) {
+ DEBUG(0, ("Invalid value for minId\n"));
+ ret = EINVAL;
+ goto done;
+ }
+
+ ret = get_entry_as_uint32(res->msgs[0], &domain->id_max,
+ "maxId", 0);
+ if (ret != EOK) {
+ DEBUG(0, ("Invalid value for maxId\n"));
+ ret = EINVAL;
+ goto done;
+ }
+
+ if (domain->id_max && (domain->id_max < domain->id_min)) {
+ DEBUG(0, ("Invalid domain range\n"));
ret = EINVAL;
goto done;
}
--
1.6.2.5
signature.asc
Description: OpenPGP digital signature
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
