From c9fcea7cae0edcc37f8d80b2f492005b970da1d2 Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 31 Jul 2023 18:00:10 +0200 Subject: [PATCH 01/15] [DCT-4]: Fix useless return in `ddsrt_chh_init` --- src/ddsrt/src/hopscotch.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/ddsrt/src/hopscotch.c b/src/ddsrt/src/hopscotch.c index b2010816e3..8899e8bb75 100644 --- a/src/ddsrt/src/hopscotch.c +++ b/src/ddsrt/src/hopscotch.c @@ -313,7 +313,7 @@ static bool ddsrt_chh_data_valid_p (void *data) return data != NULL && data != CHH_BUSY; } -static int ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) +static void ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) { uint32_t size; uint32_t i; @@ -338,7 +338,6 @@ static int ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_ha ddsrt_atomic_stvoidp (&b->data, NULL); } ddsrt_mutex_init (&rt->change_lock); - return 0; } static void ddsrt_chh_fini (struct ddsrt_chh *rt) @@ -350,12 +349,8 @@ static void ddsrt_chh_fini (struct ddsrt_chh *rt) struct ddsrt_chh *ddsrt_chh_new (uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) { struct ddsrt_chh *hh = ddsrt_malloc (sizeof (*hh)); - if (ddsrt_chh_init (hh, init_size, hash, equals, gc_buckets, gc_buckets_arg) < 0) { - ddsrt_free (hh); - return NULL; - } else { - return hh; - } + ddsrt_chh_init (hh, init_size, hash, equals, gc_buckets, gc_buckets_arg); + return hh; } void ddsrt_chh_free (struct ddsrt_chh * __restrict hh) From d1c38e3461e92787a965eb393049002a92d38f0c Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 31 Jul 2023 18:03:32 +0200 Subject: [PATCH 02/15] [DCT-5]: Fix unnecessary check on retcode in `dds_strretcode` --- src/ddsrt/src/retcode.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ddsrt/src/retcode.c b/src/ddsrt/src/retcode.c index 0efc1e4c25..f39c90ac4a 100644 --- a/src/ddsrt/src/retcode.c +++ b/src/ddsrt/src/retcode.c @@ -55,9 +55,11 @@ const char *dds_strretcode (dds_return_t ret) if (ret == INT32_MIN) return xretcodes[0]; + // INT32_MIN has already been handled and so this is safe + // and will guarantee ret >= 0. if (ret < 0) ret = -ret; - if (ret >= 0 && ret < nretcodes) + if (ret < nretcodes) return retcodes[ret]; else if (ret >= (-DDS_XRETCODE_BASE) && ret < (-DDS_XRETCODE_BASE) + nxretcodes) return xretcodes[ret - (-DDS_XRETCODE_BASE)]; From 499e930eef8062999511a30258b018208d5cf1eb Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 31 Jul 2023 18:12:23 +0200 Subject: [PATCH 03/15] [DCT-12]: Compare the `double` against `HUGE_VAL` in `strtod` --- src/ddsrt/src/strtod.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ddsrt/src/strtod.c b/src/ddsrt/src/strtod.c index 63b957b41f..b82b3ca69c 100644 --- a/src/ddsrt/src/strtod.c +++ b/src/ddsrt/src/strtod.c @@ -148,7 +148,7 @@ ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) } } - if ((dbl == HUGE_VALF || dbl == HUGE_VALL || dbl == 0) && errno == ERANGE) { + if ((dbl == HUGE_VAL || dbl == 0) && errno == ERANGE) { ret = DDS_RETCODE_OUT_OF_RANGE; } else { errno = orig_errno; From bb0d403d62ce781a1691a42fa41d4bc0235a3a3d Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 31 Jul 2023 18:22:10 +0200 Subject: [PATCH 04/15] [DCT-6]: Split up `append_to_payload` into infallible (literal) and fallible versions --- src/ddsrt/src/xmlparser.c | 45 +++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/ddsrt/src/xmlparser.c b/src/ddsrt/src/xmlparser.c index 9797b68eb1..2625913aa4 100644 --- a/src/ddsrt/src/xmlparser.c +++ b/src/ddsrt/src/xmlparser.c @@ -377,22 +377,27 @@ static void discard_payload (struct ddsrt_xmlp_state *st) st->tpescp = 0; } -static int append_to_payload (struct ddsrt_xmlp_state *st, int c, int islit) +static void append_literal_to_payload (struct ddsrt_xmlp_state *st, int c) { - if (!islit) { - st->tp[st->tpp++] = (char) c; - } else { - if (st->tpescp < st->tpp) { - size_t n = st->tpp - st->tpescp; - if (unescape_insitu (st->tp + st->tpescp, &n) < 0) { - discard_payload (st); - return -1; - } - st->tpp = st->tpescp + n; + st->tp[st->tpp++] = (char) c; + if (st->tpp == st->tpsz) { + st->tpsz += 1024; + st->tp = ddsrt_realloc (st->tp, st->tpsz); + } +} + +static int append_to_payload (struct ddsrt_xmlp_state *st, int c) +{ + if (st->tpescp < st->tpp) { + size_t n = st->tpp - st->tpescp; + if (unescape_insitu (st->tp + st->tpescp, &n) < 0) { + discard_payload (st); + return -1; } - st->tp[st->tpp++] = (char) c; - st->tpescp = st->tpp; + st->tpp = st->tpescp + n; } + st->tp[st->tpp++] = (char) c; + st->tpescp = st->tpp; if (st->tpp == st->tpsz) { st->tpsz += 1024; st->tp = ddsrt_realloc (st->tp, st->tpsz); @@ -444,9 +449,7 @@ static int save_payload (char **payload, struct ddsrt_xmlp_state *st, int trim) static int next_token_ident (struct ddsrt_xmlp_state *st, char **payload) { while (qq_isidentcont (peek_char (st))) { - if (append_to_payload (st, next_char (st), 0) < 0) { - return TOK_ERROR; - } + append_literal_to_payload(st, next_char(st)); } if (save_payload (payload, st, 0) < 0) { return TOK_ERROR; @@ -483,9 +486,7 @@ static int next_token_string (struct ddsrt_xmlp_state *st, char **payload, const { /* positioned at first character of string */ while (!peek_chars (st, endm, 0) && peek_char (st) != TOK_EOF) { - if (append_to_payload (st, next_char (st), 0) < 0) { - return TOK_ERROR; - } + append_literal_to_payload(st, next_char (st)); } if (!peek_chars (st, endm, 1)) { discard_payload (st); @@ -673,15 +674,13 @@ static int parse_element (struct ddsrt_xmlp_state *st, uintptr_t parentinfo) do { /* gobble up content until EOF or markup */ while (peek_char (st) != '<' && peek_char (st) != TOK_EOF) { - if (append_to_payload (st, next_char (st), 0) < 0) { - PE_LOCAL_ERROR ("invalid character sequence", 0); - } + append_literal_to_payload (st, next_char (st)); } /* if the mark-up happens to be a CDATA, consume it, and gobble up characters until the closing marker is reached, which then also gets consumed */ if (peek_chars (st, cdata_magic, 1)) { while (!peek_chars (st, "]]>", 1) && peek_char (st) != TOK_EOF) { - if (append_to_payload (st, next_char (st), 1) < 0) { + if (append_to_payload (st, next_char (st)) < 0) { PE_LOCAL_ERROR ("invalid character sequence", 0); } } From 8b044500163b907e58f3cbd74283accbf0c249e9 Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Tue, 1 Aug 2023 10:41:53 +0200 Subject: [PATCH 05/15] [DCT-12]: Factor out floating point string delocalization --- src/ddsrt/src/strtod.c | 50 ++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/ddsrt/src/strtod.c b/src/ddsrt/src/strtod.c index b82b3ca69c..8cf073c8b3 100644 --- a/src/ddsrt/src/strtod.c +++ b/src/ddsrt/src/strtod.c @@ -94,28 +94,17 @@ os_lcNumericReplace(char *str) { } } -dds_return_t -ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) +/** @brief Delocalize a floating point string to use '.' as its decimal separator. + * @param[in] nptr the localized floating point string. + * @param[out] nptrCopy the delocalized floating point string (needs to have a length of DOUBLE_STRING_MAX_LENGTH). + * @param[out] nptrCopyEnd a pointer to the end of the delocalized floating point string. + */ +static void delocalize_floating_point_str(const char *nptr, char *nptrCopy, char **nptrCopyEnd) { - double dbl; - int orig_errno; - dds_return_t ret = DDS_RETCODE_OK; - - assert(nptr != NULL); - assert(dblptr != NULL); - - orig_errno = errno; - if (os_lcNumericGet() == '.') { - errno = 0; - /* The current locale uses '.', so strtod can be used as is. */ - dbl = strtod(nptr, endptr); - } else { /* The current locale uses ',', so we can not use the standard functions as is, but have to do extra work because ospl uses "x.x" doubles (notice the dot). Just copy the string and replace the LC_NUMERIC. */ - char nptrCopy[DOUBLE_STRING_MAX_LENGTH]; char *nptrCopyIdx; - char *nptrCopyEnd; char *nptrIdx; /* It is possible that the given nptr just starts with a double @@ -124,8 +113,8 @@ ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) doubles' end. */ nptrIdx = (char*)nptr; nptrCopyIdx = nptrCopy; - nptrCopyEnd = nptrCopyIdx + DOUBLE_STRING_MAX_LENGTH - 1; - while (VALID_DOUBLE_CHAR(*nptrIdx) && (nptrCopyIdx < nptrCopyEnd)) { + *nptrCopyEnd = nptrCopyIdx + DOUBLE_STRING_MAX_LENGTH - 1; + while (VALID_DOUBLE_CHAR(*nptrIdx) && (nptrCopyIdx < *nptrCopyEnd)) { if (*nptrIdx == '.') { /* Replace '.' with locale LC_NUMERIC to get strtod to work. */ *nptrCopyIdx = os_lcNumericGet(); @@ -136,6 +125,29 @@ ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) nptrCopyIdx++; } *nptrCopyIdx = '\0'; +} + +dds_return_t +ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) +{ + double dbl; + int orig_errno; + dds_return_t ret = DDS_RETCODE_OK; + + assert(nptr != NULL); + assert(dblptr != NULL); + + orig_errno = errno; + if (os_lcNumericGet() == '.') { + errno = 0; + /* The current locale uses '.', so strtod can be used as is. */ + dbl = strtod(nptr, endptr); + } else { + /* The current locale has to be normalized to use '.' for the floating + point string. */ + char nptrCopy[DOUBLE_STRING_MAX_LENGTH]; + char *nptrCopyEnd = NULL; + delocalize_floating_point_str(nptr, nptrCopy, &nptrCopyEnd); /* Now that we have a copy with the proper locale LC_NUMERIC, we can use strtod() for the conversion. */ From 0ba0debd3639aaeb28794ed4b445a76007faf6ab Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Tue, 1 Aug 2023 10:42:41 +0200 Subject: [PATCH 06/15] [DCT-12]: Fix strtof to use float precision and not double precision --- src/ddsrt/src/strtod.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/ddsrt/src/strtod.c b/src/ddsrt/src/strtod.c index 8cf073c8b3..791d671282 100644 --- a/src/ddsrt/src/strtod.c +++ b/src/ddsrt/src/strtod.c @@ -174,18 +174,44 @@ ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) dds_return_t ddsrt_strtof(const char *nptr, char **endptr, float *fltptr) { - /* Just use os_strtod(). */ - /* FIXME: This will have to do for now, but a single-precision floating - point number is definitely not a double-precision floating point - number. */ - double dbl = 0.0; - dds_return_t ret; + float flt; + int orig_errno; + dds_return_t ret = DDS_RETCODE_OK; assert(nptr != NULL); assert(fltptr != NULL); - ret = ddsrt_strtod(nptr, endptr, &dbl); - *fltptr = (float)dbl; + orig_errno = errno; + if (os_lcNumericGet() == '.') { + errno = 0; + /* The current locale uses '.', so strtof can be used as is. */ + flt = strtof(nptr, endptr); + } else { + /* The current locale has to be normalized to use '.' for the floating + point string. */ + char nptrCopy[DOUBLE_STRING_MAX_LENGTH]; + char *nptrCopyEnd = NULL; + delocalize_floating_point_str(nptr, nptrCopy, &nptrCopyEnd); + + /* Now that we have a copy with the proper locale LC_NUMERIC, we can use + strtof() for the conversion. */ + errno = 0; + flt = strtof(nptrCopy, &nptrCopyEnd); + + /* Calculate the proper end char when needed. */ + if (endptr != NULL) { + *endptr = (char*)nptr + (nptrCopyEnd - nptrCopy); + } + } + + if ((flt == HUGE_VALF || flt == 0) && errno == ERANGE) { + ret = DDS_RETCODE_OUT_OF_RANGE; + } else { + errno = orig_errno; + } + + *fltptr = flt; + return ret; } From 1d7ef26ac3a0246ba5a0b7da741bfc2b61709386 Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 7 Aug 2023 10:34:30 +0200 Subject: [PATCH 07/15] Revert "[DCT-4]: Fix useless return in `ddsrt_chh_init`" This reverts commit 05a480374546e0e2c7f55438fc10329de3824c16. Based on review comments: instead of removing the error code, the underlying allocation will be switched from `ddsrt_malloc` to `ddsrt_malloc_s` which returns an error on OOM instead of aborting. --- src/ddsrt/src/hopscotch.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/ddsrt/src/hopscotch.c b/src/ddsrt/src/hopscotch.c index 8899e8bb75..b2010816e3 100644 --- a/src/ddsrt/src/hopscotch.c +++ b/src/ddsrt/src/hopscotch.c @@ -313,7 +313,7 @@ static bool ddsrt_chh_data_valid_p (void *data) return data != NULL && data != CHH_BUSY; } -static void ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) +static int ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) { uint32_t size; uint32_t i; @@ -338,6 +338,7 @@ static void ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_h ddsrt_atomic_stvoidp (&b->data, NULL); } ddsrt_mutex_init (&rt->change_lock); + return 0; } static void ddsrt_chh_fini (struct ddsrt_chh *rt) @@ -349,8 +350,12 @@ static void ddsrt_chh_fini (struct ddsrt_chh *rt) struct ddsrt_chh *ddsrt_chh_new (uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) { struct ddsrt_chh *hh = ddsrt_malloc (sizeof (*hh)); - ddsrt_chh_init (hh, init_size, hash, equals, gc_buckets, gc_buckets_arg); - return hh; + if (ddsrt_chh_init (hh, init_size, hash, equals, gc_buckets, gc_buckets_arg) < 0) { + ddsrt_free (hh); + return NULL; + } else { + return hh; + } } void ddsrt_chh_free (struct ddsrt_chh * __restrict hh) From 739da84c7de34418513b72d6cba26a9bc4944d4d Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 7 Aug 2023 11:53:21 +0200 Subject: [PATCH 08/15] [DCT-4]: Swap to `ddsrt_malloc_s` in hopscotch --- src/ddsrt/src/hopscotch.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/ddsrt/src/hopscotch.c b/src/ddsrt/src/hopscotch.c index b2010816e3..feaab42ba3 100644 --- a/src/ddsrt/src/hopscotch.c +++ b/src/ddsrt/src/hopscotch.c @@ -313,7 +313,7 @@ static bool ddsrt_chh_data_valid_p (void *data) return data != NULL && data != CHH_BUSY; } -static int ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) +static dds_return_t ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) { uint32_t size; uint32_t i; @@ -328,7 +328,10 @@ static int ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_ha rt->gc_buckets = gc_buckets; rt->gc_buckets_arg = gc_buckets_arg; - buckets = ddsrt_malloc (offsetof (struct ddsrt_chh_bucket_array, bs) + size * sizeof (*buckets->bs)); + buckets = ddsrt_malloc_s (offsetof (struct ddsrt_chh_bucket_array, bs) + size * sizeof (*buckets->bs)); + if (buckets == NULL) { + return DDS_RETCODE_OUT_OF_RESOURCES; + } ddsrt_atomic_stvoidp (&rt->buckets, buckets); buckets->size = size; for (i = 0; i < size; i++) { @@ -338,7 +341,7 @@ static int ddsrt_chh_init (struct ddsrt_chh *rt, uint32_t init_size, ddsrt_hh_ha ddsrt_atomic_stvoidp (&b->data, NULL); } ddsrt_mutex_init (&rt->change_lock); - return 0; + return DDS_RETCODE_OK; } static void ddsrt_chh_fini (struct ddsrt_chh *rt) @@ -349,8 +352,8 @@ static void ddsrt_chh_fini (struct ddsrt_chh *rt) struct ddsrt_chh *ddsrt_chh_new (uint32_t init_size, ddsrt_hh_hash_fn hash, ddsrt_hh_equals_fn equals, ddsrt_hh_buckets_gc_fn gc_buckets, void *gc_buckets_arg) { - struct ddsrt_chh *hh = ddsrt_malloc (sizeof (*hh)); - if (ddsrt_chh_init (hh, init_size, hash, equals, gc_buckets, gc_buckets_arg) < 0) { + struct ddsrt_chh *hh = ddsrt_malloc_s (sizeof (*hh)); + if (hh == NULL || ddsrt_chh_init (hh, init_size, hash, equals, gc_buckets, gc_buckets_arg) < 0) { ddsrt_free (hh); return NULL; } else { From a56bdff50b87f9c4da77a20cf0757f61107d969c Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 7 Aug 2023 12:36:10 +0200 Subject: [PATCH 09/15] [DCT-6]: Use `append_literal_to_payload` in `append_to_payload` --- src/ddsrt/src/xmlparser.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/ddsrt/src/xmlparser.c b/src/ddsrt/src/xmlparser.c index 2625913aa4..61ae5b19b3 100644 --- a/src/ddsrt/src/xmlparser.c +++ b/src/ddsrt/src/xmlparser.c @@ -396,12 +396,8 @@ static int append_to_payload (struct ddsrt_xmlp_state *st, int c) } st->tpp = st->tpescp + n; } - st->tp[st->tpp++] = (char) c; + append_literal_to_payload (st, c); st->tpescp = st->tpp; - if (st->tpp == st->tpsz) { - st->tpsz += 1024; - st->tp = ddsrt_realloc (st->tp, st->tpsz); - } return 0; } From 288964b6d904ffeea2fa4e91dba9b2c1c17f1623 Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 7 Aug 2023 15:42:08 +0200 Subject: [PATCH 10/15] Remove unused function `ddsrt_strtof` --- src/ddsrt/include/dds/ddsrt/strtod.h | 13 -------- src/ddsrt/src/strtod.c | 44 ---------------------------- 2 files changed, 57 deletions(-) diff --git a/src/ddsrt/include/dds/ddsrt/strtod.h b/src/ddsrt/include/dds/ddsrt/strtod.h index 76e12ca59d..8594da0281 100644 --- a/src/ddsrt/include/dds/ddsrt/strtod.h +++ b/src/ddsrt/include/dds/ddsrt/strtod.h @@ -38,19 +38,6 @@ extern "C" { dds_return_t ddsrt_strtod(const char *nptr, char **endptr, double *dblptr); -/** - * @brief Convert a string to a floating point number. - * - * @param[in] nptr A string to convert into a float. - * @param[in] endptr If not NULL, a char* where the address of first invalid - * character is stored. - * @param[out] fltptr A float where the floating-point number is stored. - * - * @returns A dds_return_t indicating success or failure. - */ -dds_return_t -ddsrt_strtof(const char *nptr, char **endptr, float *fltptr); - /** * @brief Convert a double-precision floating-point number to a string. * diff --git a/src/ddsrt/src/strtod.c b/src/ddsrt/src/strtod.c index 791d671282..b87d6f69d4 100644 --- a/src/ddsrt/src/strtod.c +++ b/src/ddsrt/src/strtod.c @@ -171,50 +171,6 @@ ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) return ret; } -dds_return_t -ddsrt_strtof(const char *nptr, char **endptr, float *fltptr) -{ - float flt; - int orig_errno; - dds_return_t ret = DDS_RETCODE_OK; - - assert(nptr != NULL); - assert(fltptr != NULL); - - orig_errno = errno; - if (os_lcNumericGet() == '.') { - errno = 0; - /* The current locale uses '.', so strtof can be used as is. */ - flt = strtof(nptr, endptr); - } else { - /* The current locale has to be normalized to use '.' for the floating - point string. */ - char nptrCopy[DOUBLE_STRING_MAX_LENGTH]; - char *nptrCopyEnd = NULL; - delocalize_floating_point_str(nptr, nptrCopy, &nptrCopyEnd); - - /* Now that we have a copy with the proper locale LC_NUMERIC, we can use - strtof() for the conversion. */ - errno = 0; - flt = strtof(nptrCopy, &nptrCopyEnd); - - /* Calculate the proper end char when needed. */ - if (endptr != NULL) { - *endptr = (char*)nptr + (nptrCopyEnd - nptrCopy); - } - } - - if ((flt == HUGE_VALF || flt == 0) && errno == ERANGE) { - ret = DDS_RETCODE_OUT_OF_RANGE; - } else { - errno = orig_errno; - } - - *fltptr = flt; - - return ret; -} - int ddsrt_dtostr(double src, char *str, size_t size) { From 3dcb79a64632ab6bfc7ff0685fc914983e5e3e6e Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Mon, 7 Aug 2023 17:00:44 +0200 Subject: [PATCH 11/15] Swap to `ddsrt_malloc_s` in `append_literal_to_payload` --- src/ddsrt/src/xmlparser.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/ddsrt/src/xmlparser.c b/src/ddsrt/src/xmlparser.c index 61ae5b19b3..498a71c380 100644 --- a/src/ddsrt/src/xmlparser.c +++ b/src/ddsrt/src/xmlparser.c @@ -377,13 +377,18 @@ static void discard_payload (struct ddsrt_xmlp_state *st) st->tpescp = 0; } -static void append_literal_to_payload (struct ddsrt_xmlp_state *st, int c) +static dds_return_t append_literal_to_payload (struct ddsrt_xmlp_state *st, int c) { st->tp[st->tpp++] = (char) c; if (st->tpp == st->tpsz) { st->tpsz += 1024; - st->tp = ddsrt_realloc (st->tp, st->tpsz); + st->tp = ddsrt_realloc_s (st->tp, st->tpsz); + if (st->tp == NULL) { + return DDS_RETCODE_OUT_OF_RESOURCES; + } } + + return DDS_RETCODE_OK; } static int append_to_payload (struct ddsrt_xmlp_state *st, int c) @@ -396,8 +401,10 @@ static int append_to_payload (struct ddsrt_xmlp_state *st, int c) } st->tpp = st->tpescp + n; } - append_literal_to_payload (st, c); st->tpescp = st->tpp; + if (append_literal_to_payload (st, c) < 0) { + return -1; + }; return 0; } @@ -445,7 +452,9 @@ static int save_payload (char **payload, struct ddsrt_xmlp_state *st, int trim) static int next_token_ident (struct ddsrt_xmlp_state *st, char **payload) { while (qq_isidentcont (peek_char (st))) { - append_literal_to_payload(st, next_char(st)); + if (append_literal_to_payload (st, next_char(st)) < 0) { + return TOK_ERROR; + } } if (save_payload (payload, st, 0) < 0) { return TOK_ERROR; @@ -482,7 +491,9 @@ static int next_token_string (struct ddsrt_xmlp_state *st, char **payload, const { /* positioned at first character of string */ while (!peek_chars (st, endm, 0) && peek_char (st) != TOK_EOF) { - append_literal_to_payload(st, next_char (st)); + if (append_literal_to_payload (st, next_char (st)) < 0) { + return TOK_ERROR; + } } if (!peek_chars (st, endm, 1)) { discard_payload (st); @@ -670,7 +681,9 @@ static int parse_element (struct ddsrt_xmlp_state *st, uintptr_t parentinfo) do { /* gobble up content until EOF or markup */ while (peek_char (st) != '<' && peek_char (st) != TOK_EOF) { - append_literal_to_payload (st, next_char (st)); + if (append_literal_to_payload (st, next_char (st)) < 0) { + PE_LOCAL_ERROR ("invalid character sequence", 0); + } } /* if the mark-up happens to be a CDATA, consume it, and gobble up characters until the closing marker is reached, which then also gets consumed */ From ca83435995cbec7239e59b6dddc9ace9d6e02052 Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Wed, 9 Aug 2023 07:43:43 +0200 Subject: [PATCH 12/15] [DCT-12]: Fix logic in floating point conversion --- src/ddsrt/include/dds/ddsrt/strtod.h | 9 +++++ src/ddsrt/src/strtod.c | 49 ++++++++++++++++++++++++---- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/ddsrt/include/dds/ddsrt/strtod.h b/src/ddsrt/include/dds/ddsrt/strtod.h index 8594da0281..88816c4c3b 100644 --- a/src/ddsrt/include/dds/ddsrt/strtod.h +++ b/src/ddsrt/include/dds/ddsrt/strtod.h @@ -28,6 +28,15 @@ extern "C" { /** * @brief Convert a string to a double precision floating point number. * + * This operation handles locale specific manipulation of the floating point + * string and then sets the output double based on the parsing via `strtod` + * from the standard library. The function returns a failure iff: + * - the string to be parsed represents a value that is too large to store in a double. + * - the string contains junk. + * - the string parses to either `-nan` or `nan`. + * - the string parses to either `-inf` or `inf`. + * It is otherwise successful. + * * @param[in] nptr A string to convert into a double. * @param[out] endptr If not NULL, a char* where the address of first invalid * character is stored. diff --git a/src/ddsrt/src/strtod.c b/src/ddsrt/src/strtod.c index b87d6f69d4..4c2897f72f 100644 --- a/src/ddsrt/src/strtod.c +++ b/src/ddsrt/src/strtod.c @@ -14,6 +14,7 @@ #include #include #include +#include #include "dds/ddsrt/io.h" #include "dds/ddsrt/log.h" @@ -133,6 +134,8 @@ ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) double dbl; int orig_errno; dds_return_t ret = DDS_RETCODE_OK; + char *string_end = NULL; + bool successfully_parsed = false; assert(nptr != NULL); assert(dblptr != NULL); @@ -141,26 +144,60 @@ ddsrt_strtod(const char *nptr, char **endptr, double *dblptr) if (os_lcNumericGet() == '.') { errno = 0; /* The current locale uses '.', so strtod can be used as is. */ - dbl = strtod(nptr, endptr); + dbl = strtod(nptr, &string_end); + + /* Check that something was parsed */ + if (nptr != string_end) { + successfully_parsed = true; + } + + /* Set the proper end char when needed. */ + if (endptr != NULL) { + *endptr = string_end; + } } else { /* The current locale has to be normalized to use '.' for the floating point string. */ char nptrCopy[DOUBLE_STRING_MAX_LENGTH]; - char *nptrCopyEnd = NULL; - delocalize_floating_point_str(nptr, nptrCopy, &nptrCopyEnd); + delocalize_floating_point_str(nptr, nptrCopy, &string_end); /* Now that we have a copy with the proper locale LC_NUMERIC, we can use strtod() for the conversion. */ errno = 0; - dbl = strtod(nptrCopy, &nptrCopyEnd); + dbl = strtod(nptrCopy, &string_end); + + /* Check that something was parsed */ + if (nptrCopy != string_end) { + successfully_parsed = true; + } /* Calculate the proper end char when needed. */ if (endptr != NULL) { - *endptr = (char*)nptr + (nptrCopyEnd - nptrCopy); + *endptr = (char*)nptr + (string_end - nptrCopy); } } - if ((dbl == HUGE_VAL || dbl == 0) && errno == ERANGE) { + // There are two erroring scenarios from `strtod`. + // + // 1. The floating point value to be parsed is too large: + // In this case `strtod` sets `errno` to `ERANGE` and will return a + // parsed value of either `-HUGE_VAL` or `HUGE_VAL` depending on the + // initial sign present in the string. + // + // 2. The string contains a non-numeric prefix: + // In the case junk is passed in `strtod` parses nothing. As a result, + // the value that is returned corresponds to `0.0`. To differentiate + // between the parsing scenarios of junk being passed in versus a valid + // floating point string that parses to 0 (such as "0.0") `strtod` also + // ensures that the provided end pointer == start pointer in the case + // that a junk prefix is encountered. + // + // The two other scenarios that we want to reject are: + // + // 3. The value being parsed results in `-nan` or `nan`. + // 4. The value being parsed results in `-inf` or `inf`. + if (errno == ERANGE || !successfully_parsed + || isnan(dbl) || isinf(dbl)) { ret = DDS_RETCODE_OUT_OF_RANGE; } else { errno = orig_errno; From 7eb64b604ae60ab0943883a62b7226472d211808 Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Wed, 30 Aug 2023 01:27:36 +0200 Subject: [PATCH 13/15] xmlparser: Remove erroneous `;` --- src/ddsrt/src/xmlparser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ddsrt/src/xmlparser.c b/src/ddsrt/src/xmlparser.c index 498a71c380..a16a451fc9 100644 --- a/src/ddsrt/src/xmlparser.c +++ b/src/ddsrt/src/xmlparser.c @@ -404,7 +404,7 @@ static int append_to_payload (struct ddsrt_xmlp_state *st, int c) st->tpescp = st->tpp; if (append_literal_to_payload (st, c) < 0) { return -1; - }; + } return 0; } From d4ca898b926dc625566437bb5263860515acbf9f Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Wed, 30 Aug 2023 01:31:12 +0200 Subject: [PATCH 14/15] xmlparser: Fix silly memory leak --- src/ddsrt/src/xmlparser.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ddsrt/src/xmlparser.c b/src/ddsrt/src/xmlparser.c index a16a451fc9..28045a8ab2 100644 --- a/src/ddsrt/src/xmlparser.c +++ b/src/ddsrt/src/xmlparser.c @@ -382,9 +382,11 @@ static dds_return_t append_literal_to_payload (struct ddsrt_xmlp_state *st, int st->tp[st->tpp++] = (char) c; if (st->tpp == st->tpsz) { st->tpsz += 1024; - st->tp = ddsrt_realloc_s (st->tp, st->tpsz); - if (st->tp == NULL) { + void *tp_realloc = ddsrt_realloc_s (st->tp, st->tpsz); + if (tp_realloc == NULL) { return DDS_RETCODE_OUT_OF_RESOURCES; + } else { + st->tp = tp_realloc; } } From 4f631a7bf14bac6a2b3821f4e0ca7464d58a538c Mon Sep 17 00:00:00 2001 From: Troy Karan Harrison Date: Wed, 30 Aug 2023 01:32:12 +0200 Subject: [PATCH 15/15] xmlparser: Fix off-by-one error and rename functions --- src/ddsrt/src/xmlparser.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/ddsrt/src/xmlparser.c b/src/ddsrt/src/xmlparser.c index 28045a8ab2..b9d5a72b62 100644 --- a/src/ddsrt/src/xmlparser.c +++ b/src/ddsrt/src/xmlparser.c @@ -377,7 +377,7 @@ static void discard_payload (struct ddsrt_xmlp_state *st) st->tpescp = 0; } -static dds_return_t append_literal_to_payload (struct ddsrt_xmlp_state *st, int c) +static dds_return_t append_to_payload (struct ddsrt_xmlp_state *st, int c) { st->tp[st->tpp++] = (char) c; if (st->tpp == st->tpsz) { @@ -393,7 +393,7 @@ static dds_return_t append_literal_to_payload (struct ddsrt_xmlp_state *st, int return DDS_RETCODE_OK; } -static int append_to_payload (struct ddsrt_xmlp_state *st, int c) +static int append_to_payload_no_unescape (struct ddsrt_xmlp_state *st, int c) { if (st->tpescp < st->tpp) { size_t n = st->tpp - st->tpescp; @@ -403,11 +403,15 @@ static int append_to_payload (struct ddsrt_xmlp_state *st, int c) } st->tpp = st->tpescp + n; } + int result = append_to_payload (st, c); st->tpescp = st->tpp; - if (append_literal_to_payload (st, c) < 0) { + + if (result < 0) { + discard_payload (st); return -1; + } else { + return 0; } - return 0; } static int save_payload (char **payload, struct ddsrt_xmlp_state *st, int trim) @@ -454,7 +458,7 @@ static int save_payload (char **payload, struct ddsrt_xmlp_state *st, int trim) static int next_token_ident (struct ddsrt_xmlp_state *st, char **payload) { while (qq_isidentcont (peek_char (st))) { - if (append_literal_to_payload (st, next_char(st)) < 0) { + if (append_to_payload (st, next_char(st)) < 0) { return TOK_ERROR; } } @@ -493,7 +497,7 @@ static int next_token_string (struct ddsrt_xmlp_state *st, char **payload, const { /* positioned at first character of string */ while (!peek_chars (st, endm, 0) && peek_char (st) != TOK_EOF) { - if (append_literal_to_payload (st, next_char (st)) < 0) { + if (append_to_payload (st, next_char (st)) < 0) { return TOK_ERROR; } } @@ -683,7 +687,7 @@ static int parse_element (struct ddsrt_xmlp_state *st, uintptr_t parentinfo) do { /* gobble up content until EOF or markup */ while (peek_char (st) != '<' && peek_char (st) != TOK_EOF) { - if (append_literal_to_payload (st, next_char (st)) < 0) { + if (append_to_payload (st, next_char (st)) < 0) { PE_LOCAL_ERROR ("invalid character sequence", 0); } } @@ -691,7 +695,7 @@ static int parse_element (struct ddsrt_xmlp_state *st, uintptr_t parentinfo) until the closing marker is reached, which then also gets consumed */ if (peek_chars (st, cdata_magic, 1)) { while (!peek_chars (st, "]]>", 1) && peek_char (st) != TOK_EOF) { - if (append_to_payload (st, next_char (st)) < 0) { + if (append_to_payload_no_unescape (st, next_char (st)) < 0) { PE_LOCAL_ERROR ("invalid character sequence", 0); } }