Hi Tobias,
Tobias Stoeckmann wrote on Thu, Dec 03, 2015 at 11:08:28PM +0100:
> Ingo Schwarze wrote:
>> I would suggest to use uint32_t.
> Just while applying this, I noticed that the file has a mix of the
> types u_int32_t and uint32_t. I took u_int32_t for now because it was
> in there more often than uint32_t. What do you think?
uint32_t should be preferred because that's the POSIX type, while
u_int32_t is not standardized. If you are working on the file
anyway, i'd recommend to unify all uses to uint32_t.
>> Tobias Stoeckmann wrote:
>>> +#ifndef __LP64__
>>> + if (runetype_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
>>> + maplower_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
>>> + mapupper_nranges > SIZE_MAX / sizeof(_RuneEntry))
>>> + return NULL;
>>> +#endif
>> I dislike using #ifdef, it seems error-prone to me. The checks
>> are correct in any case. Sure, if size_t is much larger than uint32_t,
>> these checks can never trigger. But why do micro-optimization here?
>> Why not just delete the #ifndef/#endif?
> If I skip this __LP64__ check, it triggers a gcc warning on amd64,
> because the compiler tells me that the test is useless. So it's a
> cosmetical thing. It's removed for now.
Oh. Frankly, i'm unsure which style is best then. Can someone
with more experience chime in to explain whether it's better
practice to ignore or avoid that warning?
If people think a preprocessor test makes sense, i'd prefer to test
for the actual property in question rather than for a macro like
__LP64__ that is overly specific and not standardized. Something
like this maybe:
#if SIZE_MAX <= UINT32_MAX
> I still cast frl_variable_len to u_int32_t for ntohl, but will cast it
> back to int32_t for a "less than 0" check. This should properly handle
> error cases when it's been negative to begin with.
True. Slightly ugly, but i don't see a better way either.
Either way, this is OK schwarze@.
Thanks again,
Ingo
> Index: lib/libc/locale/rune.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/locale/rune.c,v
> retrieving revision 1.4
> diff -u -p -u -p -r1.4 rune.c
> --- locale/rune.c 25 May 2014 17:47:04 -0000 1.4
> +++ locale/rune.c 3 Dec 2015 21:55:54 -0000
> @@ -59,32 +59,40 @@
> * SUCH DAMAGE.
> */
>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> #include <assert.h>
> +#include <errno.h>
> +#include <stdint.h>
> #include <stdio.h>
> -#include <string.h>
> #include <stdlib.h>
> -#include <errno.h>
> +#include <string.h>
> #include <wchar.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> #include "rune.h"
> #include "rune_local.h"
>
> -static int readrange(_RuneLocale *, _RuneRange *, _FileRuneRange *, void *,
> FILE *);
> +#define SAFE_ADD(x, y) \
> +do { \
> + if ((x) > SIZE_MAX - (y)) \
> + return NULL; \
> + (x) += (y); \
> +} while (0);
> +
> +static int readrange(_RuneLocale *, _RuneRange *, u_int32_t, void *, FILE *);
> static void _freeentry(_RuneRange *);
> static void _wctype_init(_RuneLocale *rl);
>
> static int
> -readrange(_RuneLocale *rl, _RuneRange *rr, _FileRuneRange *frr, void *lastp,
> +readrange(_RuneLocale *rl, _RuneRange *rr, u_int32_t nranges, void *lastp,
> FILE *fp)
> {
> - uint32_t i;
> + u_int32_t i;
> _RuneEntry *re;
> _FileRuneEntry fre;
>
> re = (_RuneEntry *)rl->rl_variable;
>
> - rr->rr_nranges = ntohl(frr->frr_nranges);
> + rr->rr_nranges = nranges;
> if (rr->rr_nranges == 0) {
> rr->rr_rune_ranges = NULL;
> return 0;
> @@ -92,6 +100,9 @@ readrange(_RuneLocale *rl, _RuneRange *r
>
> rr->rr_rune_ranges = re;
> for (i = 0; i < rr->rr_nranges; i++) {
> + if ((void *)re >= lastp)
> + return -1;
> +
> if (fread(&fre, sizeof(fre), 1, fp) != 1)
> return -1;
>
> @@ -99,9 +110,6 @@ readrange(_RuneLocale *rl, _RuneRange *r
> re->re_max = ntohl((u_int32_t)fre.fre_max);
> re->re_map = ntohl((u_int32_t)fre.fre_map);
> re++;
> -
> - if ((void *)re > lastp)
> - return -1;
> }
> rl->rl_variable = re;
> return 0;
> @@ -121,6 +129,9 @@ readentry(_RuneRange *rr, FILE *fp)
> continue;
> }
>
> + if (re[i].re_max < re[i].re_min)
> + goto fail;
> +
> l = re[i].re_max - re[i].re_min + 1;
> re[i].re_rune_types = calloc(l, sizeof(_RuneType));
> if (!re[i].re_rune_types) {
> @@ -151,17 +162,20 @@ fail2:
> }
>
> /* XXX: temporary implementation */
> -static void
> +static int
> find_codeset(_RuneLocale *rl)
> {
> char *top, *codeset, *tail, *ep;
>
> + if (rl->rl_variable == NULL)
> + return 0;
> +
> /* end of rl_variable region */
> ep = (char *)rl->rl_variable;
> ep += rl->rl_variable_len;
> rl->rl_codeset = NULL;
> if (!(top = strstr(rl->rl_variable, _RUNE_CODESET)))
> - return;
> + return 0;
> tail = strpbrk(top, " \t");
> codeset = top + sizeof(_RUNE_CODESET) - 1;
> if (tail) {
> @@ -173,18 +187,18 @@ find_codeset(_RuneLocale *rl)
> *top = '\0';
> rl->rl_codeset = strdup(codeset);
> }
> + return (rl->rl_codeset == NULL);
> }
>
> void
> _freeentry(_RuneRange *rr)
> {
> _RuneEntry *re;
> - uint32_t i;
> + u_int32_t i;
>
> re = rr->rr_rune_ranges;
> for (i = 0; i < rr->rr_nranges; i++) {
> - if (re[i].re_rune_types)
> - free(re[i].re_rune_types);
> + free(re[i].re_rune_types);
> re[i].re_rune_types = NULL;
> }
> }
> @@ -209,6 +223,7 @@ _Read_RuneMagi(FILE *fp)
> _RuneLocale *rl;
> struct stat sb;
> int x;
> + u_int32_t runetype_nranges, maplower_nranges, mapupper_nranges, var_len;
>
> if (fstat(fileno(fp), &sb) < 0)
> return NULL;
> @@ -225,10 +240,22 @@ _Read_RuneMagi(FILE *fp)
> if (memcmp(frl.frl_magic, _RUNE_MAGIC_1, sizeof(frl.frl_magic)))
> return NULL;
>
> - hostdatalen = sizeof(*rl) + ntohl((u_int32_t)frl.frl_variable_len) +
> - ntohl(frl.frl_runetype_ext.frr_nranges) * sizeof(_RuneEntry) +
> - ntohl(frl.frl_maplower_ext.frr_nranges) * sizeof(_RuneEntry) +
> - ntohl(frl.frl_mapupper_ext.frr_nranges) * sizeof(_RuneEntry);
> + runetype_nranges = ntohl(frl.frl_runetype_ext.frr_nranges);
> + maplower_nranges = ntohl(frl.frl_maplower_ext.frr_nranges);
> + mapupper_nranges = ntohl(frl.frl_mapupper_ext.frr_nranges);
> + var_len = ntohl((u_int32_t)frl.frl_variable_len);
> +
> + if (runetype_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
> + maplower_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
> + mapupper_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
> + (int32_t)var_len < 0)
> + return NULL;
> +
> + hostdatalen = sizeof(*rl);
> + SAFE_ADD(hostdatalen, var_len);
> + SAFE_ADD(hostdatalen, runetype_nranges * sizeof(_RuneEntry));
> + SAFE_ADD(hostdatalen, maplower_nranges * sizeof(_RuneEntry));
> + SAFE_ADD(hostdatalen, mapupper_nranges * sizeof(_RuneEntry));
>
> if ((hostdata = calloc(hostdatalen, 1)) == NULL)
> return NULL;
> @@ -240,6 +267,7 @@ _Read_RuneMagi(FILE *fp)
> memcpy(rl->rl_magic, frl.frl_magic, sizeof(rl->rl_magic));
> memcpy(rl->rl_encoding, frl.frl_encoding, sizeof(rl->rl_encoding));
>
> + /* XXX assumes rune_t = u_int32_t */
> rl->rl_invalid_rune = ntohl((u_int32_t)frl.frl_invalid_rune);
> rl->rl_variable_len = ntohl((u_int32_t)frl.frl_variable_len);
>
> @@ -251,54 +279,39 @@ _Read_RuneMagi(FILE *fp)
> rl->rl_mapupper[x] = ntohl((u_int32_t)frl.frl_mapupper[x]);
> }
>
> - if (readrange(rl, &rl->rl_runetype_ext, &frl.frl_runetype_ext, lastp,
> fp))
> - {
> - free(hostdata);
> - return NULL;
> - }
> - if (readrange(rl, &rl->rl_maplower_ext, &frl.frl_maplower_ext, lastp,
> fp))
> - {
> - free(hostdata);
> - return NULL;
> - }
> - if (readrange(rl, &rl->rl_mapupper_ext, &frl.frl_mapupper_ext, lastp,
> fp))
> - {
> - free(hostdata);
> - return NULL;
> - }
> + if (readrange(rl, &rl->rl_runetype_ext, runetype_nranges, lastp, fp) ||
> + readrange(rl, &rl->rl_maplower_ext, maplower_nranges, lastp, fp) ||
> + readrange(rl, &rl->rl_mapupper_ext, mapupper_nranges, lastp, fp))
> + goto err;
>
> - if (readentry(&rl->rl_runetype_ext, fp) != 0) {
> - free(hostdata);
> - return NULL;
> - }
> + if (readentry(&rl->rl_runetype_ext, fp) != 0)
> + goto err;
>
> if ((u_int8_t *)rl->rl_variable + rl->rl_variable_len >
> - (u_int8_t *)lastp) {
> - _freeentry(&rl->rl_runetype_ext);
> - free(hostdata);
> - return NULL;
> - }
> + (u_int8_t *)lastp)
> + goto rune_err;
> +
> if (rl->rl_variable_len == 0)
> rl->rl_variable = NULL;
> - else if (fread(rl->rl_variable, rl->rl_variable_len, 1, fp) != 1) {
> - _freeentry(&rl->rl_runetype_ext);
> - free(hostdata);
> - return NULL;
> - }
> - find_codeset(rl);
> + else if (fread(rl->rl_variable, rl->rl_variable_len, 1, fp) != 1)
> + goto rune_err;
> + if (find_codeset(rl))
> + goto rune_err;
> _wctype_init(rl);
>
> /*
> * error if we have junk at the tail,
> * or if we can't allocate memory.
> */
> - if (ftell(fp) != sb.st_size || __make_ctype_tabs(rl) == -1) {
> - _freeentry(&rl->rl_runetype_ext);
> - free(hostdata);
> - return NULL;
> - }
> + if (ftello(fp) != sb.st_size || __make_ctype_tabs(rl) == -1)
> + goto rune_err;
>
> return(rl);
> +rune_err:
> + _freeentry(&rl->rl_runetype_ext);
> +err:
> + free(hostdata);
> + return NULL;
> }
>
> void
