[libblockdev 1/3] Omit always true condition and document some invariants.

Vratislav Podzimek vpodzime at redhat.com
Thu Sep 4 07:23:08 UTC 2014


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 at 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

-- 
Vratislav Podzimek

Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic




More information about the anaconda-patches mailing list