One more patch. I simplified the logic (don't need to check for overflow or underflow because if checking whether it's larger than INT32_MAX is sufficient.
Also modified it so that it always passes errno back from strtoll unless
we manually set it to ERANGE.
Finally, modified the use of the function in the confdb to always test
for errno.
On 09/10/2009 04:57 PM, Stephen Gallagher wrote:
> 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.
>>
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Stephen Gallagher
RHCE 804006346421761
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From 41e668c8ebf82b92d0f596f96e1df919bdbc12b2 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 | 65 ++++++++++++++++++++++++++++++++++++++++++++++
server/util/strtonum.h | 32 ++++++++++++++++++++++
5 files changed, 144 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..744e0f7
--- /dev/null
+++ b/server/util/strtonum.c
@@ -0,0 +1,65 @@
+/*
+ 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 > INT32_MAX) {
+ errno = ERANGE;
+ return INT32_MAX;
+ }
+ else if (ret < INT32_MIN) {
+ errno = ERANGE;
+ return INT32_MIN;
+ }
+
+ /* If errno was set by strtoll, we'll pass it back as-is */
+ 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 > UINT32_MAX) {
+ errno = ERANGE;
+ return UINT32_MAX;
+ }
+
+ /* If errno was set by strtoll, we'll pass it back as-is */
+ 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 f55d1b3aa50c5668dded813765c9eae03ad1202b 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 | 59 +++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 53 insertions(+), 6 deletions(-)
diff --git a/server/confdb/confdb.c b/server/confdb/confdb.c
index 778345f..1ef2233 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,39 @@ 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;
+ }
+
+ u32ret = strtouint32 (tmp, &endptr, 10);
+ 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 +815,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
