Permalink
Browse files

I'm not entirely happy with Xi Wang's solution to this mess.

The cast to (unsigned) feels like we're just exploiting
a compiler limitation.
The original idea here was to detect overflow and therefore
avoid hard-coding assumptions about the platform arithmetic.
But allowing compilers to assume that overflow can never happen
means that portable C code can never really detect overflow
without hackish workarounds.

The only practical solution seems to be an arbitrary limit
on the size of a string that we can format.
  • Loading branch information...
1 parent ff1e307 commit ce39cb40953a110eafe6c80b292e545ffa2e7963 @kientzle kientzle committed Sep 29, 2012
Showing with 6 additions and 11 deletions.
  1. +6 −11 tar/util.c
View
@@ -120,16 +120,12 @@ safe_fprintf(FILE *f, const char *fmt, ...)
fmtbuff_length = length+1;
else if (fmtbuff_length < 8192)
fmtbuff_length *= 2;
+ else if (fmtbuff_length < 1000000)
+ fmtbuff_length += fmtbuff_length / 4;
else {
- int old_length = fmtbuff_length;
- /* Convert to unsigned to avoid signed overflow,
- * otherwise the check may be optimized away. */
- fmtbuff_length += (unsigned)fmtbuff_length / 4;
- if (old_length > fmtbuff_length) {
- length = old_length;
- fmtbuff_heap[length-1] = '\0';
- break;
- }
+ length = old_length;
+ fmtbuff_heap[length-1] = '\0';
+ break;
}
free(fmtbuff_heap);
fmtbuff_heap = malloc(fmtbuff_length);
@@ -153,8 +149,7 @@ safe_fprintf(FILE *f, const char *fmt, ...)
if (mbtowc(NULL, NULL, 1) == -1) { /* Reset the shift state. */
/* NOTE: This case may not happen, but it needs to be compiled
* safely without warnings by both gcc on linux and clang. */
- if (fmtbuff_heap != NULL)
- free(fmtbuff_heap);
+ free(fmtbuff_heap);
return;
}

0 comments on commit ce39cb4

Please sign in to comment.