Re: [Bacula-devel] bsnprintf question

[Kern S. <ke...@si...>]

Hello Radoslaw,

Sorry for the delay in my response, and thanks for your detailed
explanation of the bug and for having found the bug.  With your
explanation, I now understand what is going wrong.

It will be fixed in the next version of Bacula.

Thanks for submitting this and taking the time to explain it.

Best regards,
Kern

On 02/10/2013 02:37 PM, Radosław Korzeniewski wrote:
> Hello Kern,
> 
> 2013/2/9 Kern Sibbald <ker...@ba...
> <mailto:ker...@ba...>>
> 
>     Hello Radoslaw,
> 
>     Originally we did not use floating point output, because it was very
>     badly broken -- it seg faulted all the time.  Somewhat later, I enabled
>     it so we could use in a few places where we needed decimal numbers.
> 
>     The routine could well be broken, I simply worked on it until it seemed
>     to work correctly, but I did not try out all cases.  One thing to check
>     and to be careful about is whether you are giving it a 32 bit value or a
>     64 bit value.  Our conventions do not at all flow the stupid system
>     dependent conventions of glibc. I may have used only 64 bit floating
>     numbers.
> 
> 
> I understand the history and legacy of the code. I proposed a simple
> patch which fix this particular incorrectness. You can use it if you want.
>  
> 
> 
>     I can see your patch, and I can understand the logic of the
>     original, but I don't understand why the original is wrong or why
>     yours works.
> 
> 
> The logic of the original code (the same logic for conversion integer
> and fractional part of the floating point number) is working only for
> integer part of the conversion and is not working for the fractional one
> because leading zeros are obsolete for integer part (and original logic
> simply ignores them) and very, very important for the fractional one =
> required precision.
> 
> For example, a floating point number: 1.05 with precision 2 (max=2) is
> splited into two parts: intpart = 1 and fractional one fracpart = 5 in
> below code:
> 
> 691:    intpart = (int64_t)ufvalue;
> (...)
> 700:    /* We "cheat" by converting the fractional part to integer by
> 701:    * multiplying by a factor of 10
> 702:    */
> 703:   fracpart = round((pow10(max)) * (ufvalue - intpart));
> 
> With above variables the main conversion loop:
> 
> 727: /* Convert fractional part */
> 728: cvt_str = caps ? "0123456789ABCDEF" : "0123456789abcdef";
> 729: do {
> 730:   fconvert[fplace++] = cvt_str[fracpart % 10];
> above stores '5' in fconvert [0] in first loop, which is ok
> 
> 731:   fracpart = (fracpart / 10);
> fracpart is set to zero (int)(5/10) = 0
> 
> 732: } while (fracpart && (fplace < (int)sizeof(fconvert)));
> and above loop condition (fracpart) forces loop to finish - we had only
> one conversion loop but requirement was for two...
> My patch forces two loop runs which allow to required precision and
> leading zero to hit the conversion buffer.
> 
> Our variables after conversion loop in original code:
> fracpart = 0
> fconvert[] = "5"
> fplace = 1
> max = 2
> 
> Variables after conversion loop in patched code:
> fracpart = 0
> fconvert[] = "50"
> fplace = 2
> max = 2
> 
> Now the following loop is filling output buffer for fractional part of
> the conversion:
> 
> 841:   /*
> 842:    * Decimal point.  This should probably use locale to find the
> correct
> 843:    * char to print out.
> 844:    */
> 845:   if (max > 0) {
> 846:      outch('.');
> 847:      while (fplace > 0) {
> 848:         fplace--;
> 849:         outch(fconvert[fplace]);
> 850:      }
> 851:   }
> 
> which add string: ".5" to the output in original code. Full output will
> be then: "1.5" which is incorrect because our fp number was: 1.05. With
> my patch the above loop will produce a string ".05" and full output will
> be then "1.05", which is correct and expected.
> 
> I hope this explains the original code logic and my patch.
> 
> best regards
> -- 
> Radosław Korzeniewski
> rad...@ko... <mailto:rad...@ko...>