Patches are all either new or different from previous versions.
I took a look at this code because of the Coverity error report about it: Error: OVERRUN (CWE-119): src/utils/sizes.c:40: cond_const: Checking "i < 7" implies that "i" has the value 7 on the false branch. src/utils/sizes.c:51: overrun-local: Overrunning array "size_prefixes" of 7 8-byte elements at element index 7 (byte offset 56) using index "i" (which evaluates to 7).
mulhern (3): Omit always true condition and document some invariants. Add a test for the maximum representable size. Omit cast based distinction and print all as floats
src/utils/sizes.c | 19 ++++++++++++++----- src/utils/test_sizes.c | 4 ++++ 2 files changed, 18 insertions(+), 5 deletions(-)
i < NUM_PREFIXES is always true in the for-statement.
It should be omitted not for performance reasons but because it is likely to cause the misguided programmer to pursue the following train of thought:
That i < NUM_PREFIXES condition must be there for a reason. But the only reason for it to be there as part of the conjunction is because there is some condition under which it is false. In that case, there is some condition under which i is equal to 7 when the loop is exited. In that case, it is possible for i to be 7 when size_prefixes[i] is evaluated (unless 10 is always greater than value when i is 7 which seems highly unlikely). But size_prefixes only has 7 elements. That means that the code can access outside the bounds of the array, which is a bug. Oh no, what do we do now?
The invariants are significant ones since it is they that ensure that an out-of-bounds access can not possibly occur.
Omitting the deceptive condition and documenting the significant invariants should prevent any subsequent programmers from even temporarily following the incorrect train of thought described above.
Signed-off-by: mulhern amulhern@redhat.com --- src/utils/sizes.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/utils/sizes.c b/src/utils/sizes.c index d4eb622..91e2713 100644 --- a/src/utils/sizes.c +++ b/src/utils/sizes.c @@ -37,16 +37,28 @@ gchar* bd_utils_size_human_readable (guint64 size) { gdouble value = (gdouble) size; gdouble prev_value = (gdouble) value;
- for (i=0; i < NUM_PREFIXES && INT_LT_FLOAT(1024, value); i++) { + for (i=0; INT_LT_FLOAT(1024, value); i++) { prev_value = value; value = value / 1024.0; }
+ /* + * Since the type of size is guint64, its maximum value is 2^64 - 1. + * ((2^64 - 1) / 1024^6) == 15 which is less than 1024. Consequently, + * the loop above must terminate after at most six iterations and the + * maximum value of i at this point is 6. + */ + if (INT_GT_FLOAT(10, value)) { value = prev_value; i = i - 1; }
+ /* + * At this point the maximum value of i is 6 so any size_prefixes accesses + * below are guaranteed to remain within bounds. + */ + if (INT_EQ_FLOAT (value, (guint64) value)) return g_strdup_printf ("%"G_GUINT64_FORMAT" %sB", (guint64) value, size_prefixes[i]); else
On Wed, 2014-09-03 at 11:41 -0400, mulhern wrote:
i < NUM_PREFIXES is always true in the for-statement.
It should be omitted not for performance reasons but because it is likely to cause the misguided programmer to pursue the following train of thought:
That i < NUM_PREFIXES condition must be there for a reason. But the only reason for it to be there as part of the conjunction is because there is some condition under which it is false. In that case, there is some condition under which i is equal to 7 when the loop is exited. In that case, it is possible for i to be 7 when size_prefixes[i] is evaluated (unless 10 is always greater than value when i is 7 which seems highly unlikely). But size_prefixes only has 7 elements. That means that the code can access outside the bounds of the array, which is a bug. Oh no, what do we do now?
The invariants are significant ones since it is they that ensure that an out-of-bounds access can not possibly occur.
Omitting the deceptive condition and documenting the significant invariants should prevent any subsequent programmers from even temporarily following the incorrect train of thought described above.
Now I'm really interested in doing a survey with the code above to see how many programmers follow train of thought described above. I must say I don't have a dedicated coverity part of my brain so I'm not that kind of programmer and I'd be concerning about the missing check of boundaries in that statement as I consider that a common practice when iterating over an array in C.
Signed-off-by: mulhern amulhern@redhat.com
src/utils/sizes.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/utils/sizes.c b/src/utils/sizes.c index d4eb622..91e2713 100644 --- a/src/utils/sizes.c +++ b/src/utils/sizes.c @@ -37,16 +37,28 @@ gchar* bd_utils_size_human_readable (guint64 size) { gdouble value = (gdouble) size; gdouble prev_value = (gdouble) value;
- for (i=0; i < NUM_PREFIXES && INT_LT_FLOAT(1024, value); i++) {
for (i=0; INT_LT_FLOAT(1024, value); i++) { prev_value = value; value = value / 1024.0; }
/*
* Since the type of size is guint64, its maximum value is 2^64 - 1.
* ((2^64 - 1) / 1024^6) == 15 which is less than 1024. Consequently,
* the loop above must terminate after at most six iterations and the
* maximum value of i at this point is 6.
*/
Please move this comment above the 'for' statement so that people like me don't have to think about the missing boundary check for some time before they (we) notice, read and understand the comment.
if (INT_GT_FLOAT(10, value)) { value = prev_value; i = i - 1; }
/*
* At this point the maximum value of i is 6 so any size_prefixes accesses
* below are guaranteed to remain within bounds.
*/
if (INT_EQ_FLOAT (value, (guint64) value)) return g_strdup_printf ("%"G_GUINT64_FORMAT" %sB", (guint64) value, size_prefixes[i]); else
Signed-off-by: mulhern amulhern@redhat.com --- src/utils/test_sizes.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/utils/test_sizes.c b/src/utils/test_sizes.c index 805cd40..064df90 100644 --- a/src/utils/test_sizes.c +++ b/src/utils/test_sizes.c @@ -20,6 +20,10 @@ int main () { human = bd_utils_size_human_readable ((16.4356 GiB)); puts (human); g_free(human); + /* G_MAXUINT64 = 2^64 - 1 which is less by one than 16 EiB. */ + human = bd_utils_size_human_readable (G_MAXUINT64); + puts (human); + g_free(human);
g_printf ("10 KiB == %"G_GUINT64_FORMAT"\n", bd_utils_size_from_spec ("10 KiB", &error_message)); if (error_message)
On Wed, 2014-09-03 at 11:41 -0400, mulhern wrote:
Signed-off-by: mulhern amulhern@redhat.com
src/utils/test_sizes.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/utils/test_sizes.c b/src/utils/test_sizes.c index 805cd40..064df90 100644 --- a/src/utils/test_sizes.c +++ b/src/utils/test_sizes.c @@ -20,6 +20,10 @@ int main () { human = bd_utils_size_human_readable ((16.4356 GiB)); puts (human); g_free(human);
/* G_MAXUINT64 = 2^64 - 1 which is less by one than 16 EiB. */
human = bd_utils_size_human_readable (G_MAXUINT64);
puts (human);
g_free(human);
g_printf ("10 KiB == %"G_GUINT64_FORMAT"\n", bd_utils_size_from_spec ("10 KiB", &error_message));
if (error_message)
ACK.
It just seems wrong to claim that 16EiB - 1 is exactly 16EiB, which is what we claim when we leave off the decimal point, when it is only very close, i.e., within 1/100 of an EiB, easily.
Signed-off-by: mulhern amulhern@redhat.com --- src/utils/sizes.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/utils/sizes.c b/src/utils/sizes.c index 91e2713..9a5ec49 100644 --- a/src/utils/sizes.c +++ b/src/utils/sizes.c @@ -59,10 +59,7 @@ gchar* bd_utils_size_human_readable (guint64 size) { * below are guaranteed to remain within bounds. */
- if (INT_EQ_FLOAT (value, (guint64) value)) - return g_strdup_printf ("%"G_GUINT64_FORMAT" %sB", (guint64) value, size_prefixes[i]); - else - return g_strdup_printf ("%.2f %sB", value, size_prefixes[i]); + return g_strdup_printf ("%.2f %sB", value, size_prefixes[i]); }
/**
On Wed, 2014-09-03 at 11:41 -0400, mulhern wrote:
It just seems wrong to claim that 16EiB - 1 is exactly 16EiB, which is what we claim when we leave off the decimal point, when it is only very close, i.e., within 1/100 of an EiB, easily.
Signed-off-by: mulhern amulhern@redhat.com
src/utils/sizes.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/utils/sizes.c b/src/utils/sizes.c index 91e2713..9a5ec49 100644 --- a/src/utils/sizes.c +++ b/src/utils/sizes.c @@ -59,10 +59,7 @@ gchar* bd_utils_size_human_readable (guint64 size) { * below are guaranteed to remain within bounds. */
- if (INT_EQ_FLOAT (value, (guint64) value))
return g_strdup_printf ("%"G_GUINT64_FORMAT" %sB", (guint64) value, size_prefixes[i]);
- else
return g_strdup_printf ("%.2f %sB", value, size_prefixes[i]);
- return g_strdup_printf ("%.2f %sB", value, size_prefixes[i]);
}
NACK. We should keep the blivet's Size.humanReadable conventions which, among the other things, implies showing integer values as integers not floats. I you're really worried about 16 EiB - 1 (a value that I'm sure can only appear in artificial test cases) being wrongly shown as 16 EiB, then please find a better solution to the issue.
anaconda-patches@lists.fedorahosted.org