New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curl_mfprintf precision parameter isn't working right for floating point #1113

Closed
jay opened this Issue Nov 8, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@jay
Member

jay commented Nov 8, 2016

I did this

curl_mfprintf(stdout, "%.*f", 6, 2.277000);

I expected the following

2.277000

Instead I got

2.3

Walkthrough

In dprintf_formatf after the prescan the precision is set here:

    /* pick up the specified precision */
    if(p->flags & FLAGS_PRECPARAM) {
      prec = (long)vto[p->precision].data.num.as_signed;
      param_num++; /* since the precision is extracted from a parameter, we
                      must skip that to get to the next one properly */
      if(prec < 0)
        /* "A negative precision is taken as if the precision were
           omitted." */
        prec = -1;
    }
    else if(p->flags & FLAGS_PREC)
      prec = p->precision;
    else
      prec = -1;

p->flags is FLAGS_PREC | FLAGS_PRECPARAM,
p->precision is 1,
and so the precision is correctly set from the parameter as 6.

Next the specific type processing is done for FORMAT_DOUBLE and it resets both width and precision, as seen here:

        prec = -1;
        if(p->flags & FLAGS_PREC)
          prec = p->precision;
        else if(p->flags & FLAGS_PRECPARAM)
          prec = (long)vto[p->precision].data.num.as_signed;

p->flags and p->precision are unchanged so prec is incorrectly set as 1 and that's what causes the output to be rounded to 2.3. I'm baffled by the different processing at this point. It looks like width may have a similar issue.

curl/libcurl version

master 2016-11-08 curl-7_51_0-22-g5e6c04f

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 8, 2016

Member

I'm extending test 557 right now and will add some test strings for this as well and then we can get a patch going.

Member

bagder commented Nov 8, 2016

I'm extending test 557 right now and will add some test strings for this as well and then we can get a patch going.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 8, 2016

Member

(removed a comment I made based on me not reading correctly)

Member

bagder commented Nov 8, 2016

(removed a comment I made based on me not reading correctly)

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 8, 2016

Member

Remove a single line seems to fix it (I have a larger amount of float tests to add to also make sure we don't regress).

diff --git a/lib/mprintf.c b/lib/mprintf.c
index 2c88aa8..2af3d33 100644
--- a/lib/mprintf.c
+++ b/lib/mprintf.c
@@ -301,11 +301,10 @@ static int dprintf_Pass1(const char *format, va_stack_t *vto, char **endpos,
           break;
         case '#':
           flags |= FLAGS_ALT;
           break;
         case '.':
-          flags |= FLAGS_PREC;
           if('*' == *fmt) {
             /* The precision is picked from a specified parameter */

             flags |= FLAGS_PRECPARAM;
             fmt++;
Member

bagder commented Nov 8, 2016

Remove a single line seems to fix it (I have a larger amount of float tests to add to also make sure we don't regress).

diff --git a/lib/mprintf.c b/lib/mprintf.c
index 2c88aa8..2af3d33 100644
--- a/lib/mprintf.c
+++ b/lib/mprintf.c
@@ -301,11 +301,10 @@ static int dprintf_Pass1(const char *format, va_stack_t *vto, char **endpos,
           break;
         case '#':
           flags |= FLAGS_ALT;
           break;
         case '.':
-          flags |= FLAGS_PREC;
           if('*' == *fmt) {
             /* The precision is picked from a specified parameter */

             flags |= FLAGS_PRECPARAM;
             fmt++;

bagder added a commit that referenced this issue Nov 8, 2016

printf: fix ".*f" handling
It would always use precision 1 instead of reading it from the argument
list as intended.

Reported-by: Ray Satiro

Bug: #1113
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 8, 2016

Member

I decided to push it. I figured you could try it out and if it fixes the issue for you as well, we can close this as fixed.

Member

bagder commented Nov 8, 2016

I decided to push it. I figured you could try it out and if it fixes the issue for you as well, we can close this as fixed.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 8, 2016

Member

While the patch may be correct I still don't get it. Before type specific processing is done the width and the precision are obtained, which makes sense, because that way they can be used for any type. But then why is it overridden for float type? Is it possible that's some artifact that was supposed to be removed?

Member

jay commented Nov 8, 2016

While the patch may be correct I still don't get it. Before type specific processing is done the width and the precision are obtained, which makes sense, because that way they can be used for any type. But then why is it overridden for float type? Is it possible that's some artifact that was supposed to be removed?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 8, 2016

Member

I wouldn't call it "overridden". There are two separate code paths, one that output integers and one that deals with floats, so width and precision are handled in two places. The code path for floats checks the flags in the other order so it is sensitive to FLAGS_PREC being set together with FLAGS_PRECPARAM while the integer code path is not. Those flags are mutually exclusive anyway.

Member

bagder commented Nov 8, 2016

I wouldn't call it "overridden". There are two separate code paths, one that output integers and one that deals with floats, so width and precision are handled in two places. The code path for floats checks the flags in the other order so it is sensitive to FLAGS_PREC being set together with FLAGS_PRECPARAM while the integer code path is not. Those flags are mutually exclusive anyway.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 10, 2016

Member

But why. If the bug is the flags are supposed to be mutually exclusive then it's almost the same logic in both places. It looks wrong, the main prec/width logic accounts for negative values and the float prec/width logic does away with that. Try curl_mfprintf(stderr, "%*f", -12, 3.1); it doesn't set the width. I do see one difference I can't explain, if not FLAGS_WIDTHPARAM then the main logic will set width = p->width even if not FLAGS_WIDTH. However in float logic if there's no FLAGS_WIDTH then width is -1

Member

jay commented Nov 10, 2016

But why. If the bug is the flags are supposed to be mutually exclusive then it's almost the same logic in both places. It looks wrong, the main prec/width logic accounts for negative values and the float prec/width logic does away with that. Try curl_mfprintf(stderr, "%*f", -12, 3.1); it doesn't set the width. I do see one difference I can't explain, if not FLAGS_WIDTHPARAM then the main logic will set width = p->width even if not FLAGS_WIDTH. However in float logic if there's no FLAGS_WIDTH then width is -1

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 10, 2016

Member

You're obviously now talking about other things than this particular issue. I will not deny that the mprintf() function can be improved, but I'm saying that my landed commit fixes the issues you reported.

The not handling of negative width seems like a bug for example.

But note: A) this function is deprecated and we strongly advice people not to use it in programs and B) we don't use such features in curl or libcurl. I'm not that concerned, but if you feel like it, do add more test cases and fix remaining bugs.

Member

bagder commented Nov 10, 2016

You're obviously now talking about other things than this particular issue. I will not deny that the mprintf() function can be improved, but I'm saying that my landed commit fixes the issues you reported.

The not handling of negative width seems like a bug for example.

But note: A) this function is deprecated and we strongly advice people not to use it in programs and B) we don't use such features in curl or libcurl. I'm not that concerned, but if you feel like it, do add more test cases and fix remaining bugs.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 10, 2016

Member

Well I'm talking about something other than your solution but I think it's related. Fair enough though I will investigate more and start a new issue.

Member

jay commented Nov 10, 2016

Well I'm talking about something other than your solution but I think it's related. Fair enough though I will investigate more and start a new issue.

@jay jay closed this Nov 10, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.