[Bug 1001490] New: C99 snprintf() does not include terminated null in truncated strings

[Bug 1001490] New: C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

           Summary: C99 snprintf() does not include terminated null in
                    truncated strings
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Other
            Status: NEW
          Severity: enhancement
          Priority: low
         Component: Patches and contributions
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: sergei.gavrikov@gmail.com
                CC: ecos-patches@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1587)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1587)
add null terminator for truncated strings;  fix checks in stdio sprintf2.c test

SNPRINTF(3)

        The functions snprintf() and vsnprintf() do not write more
        than size bytes (including the terminating null byte ('\0')). If the
        output was truncated due to this limit then the return value is the
        number of characters (excluding the terminating null byte)
        which would have been written to the final string if enough
        space had been available. Thus, a return value of size or more
        means that the output was truncated.

But, truncated string of eCos "C99" snprintf() does not include the
terminating null byte ('\0').

You would see this bug running eCos stdio `sprintf2' test, if

  cdl_option CYGIMP_LIBC_STDIO_C99_SNPRINTF {
      user_value 1
  };

applied. For example, this is a case when you add eCos USTL package
which claims CYGIMP_LIBC_STDIO_C99_SNPRINTF.

Then a built `sprintf2' test (it uses a few checks for snprintf()) fails on

  FAIL:<simple snprintf test #1> Line: 138, File:
language/c/libc/stdio/current/tests/sprintf2.c
  FAIL:<simple snprintf test #1 return code> Line: 140, File:
language/c/libc/stdio/current/tests/sprintf2.c
  FAIL:<simple snprintf test #2> Line: 145, File:
language/c/libc/stdio/current/tests/sprintf2.c
  FAIL:<simple snprintf test #2 return code> Line: 147, File:
language/c/libc/stdio/current/tests/sprintf2.c
  FAIL:<snprintf double test #1> Line: 169, File:
language/c/libc/stdio/current/tests/sprintf2.c
  FAIL:<snprintf double test #1 return code> Line: 172, File:
language/c/libc/stdio/current/tests/sprintf2.c
  EXIT:<Finished tests from testcase
language/c/libc/stdio/current/tests/sprintf2.c for C library sprintf()
function>

The fails on checks the 'return code' (lines 140, 147, and 172) are okay
due C99 snprintf() return value.

But checks on lines 138, 145, and 169 are "strcmp()" checks and they
should not fail if snprintf() implemented properly.

Workaround is to add additional check and set the terminator in eCos
vsnprintf.cxx, see an attached patch. Also the patch adds proper checks
for return codes in sprintf2.c test if C99 implementation snprintf()
uses. With applied patch all snprintf() checks pass as we could expect.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

Andreas Stenius <ecos@astekk.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ecos@astekk.se

--- Comment #1 from Andreas Stenius <ecos@astekk.se> 2012-06-27 19:37:09 BST ---
I have verified the issue, and that the attached patch solves it.

However, I have not run the affected ecos tests in this patch (Although I have
run the test suite for our project).

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

Bernd Edlinger <bernd.edlinger@hotmail.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bernd.edlinger@hotmail.de

--- Comment #2 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-07 11:05:39 BST ---
(In reply to comment #1)
> I have verified the issue, and that the attached patch solves it.
> However, I have not run the affected ecos tests in this patch (Although I have
> run the test suite for our project).

Note: the test case as it was attached fails in "snprintf double test #2",
which has to be changed like the other test cases.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #3 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2012-08-07 17:44:01 BST ---
(In reply to comment #2)
> > I have verified the issue, and that the attached patch solves it.
> > However, I have not run the affected ecos tests in this patch
> > (Although I have run the test suite for our project).
> 
> Note: the test case as it was attached fails in "snprintf double test
> #2", which has to be changed like the other test cases.

Thank you for testing. But I do not see any "snprintf double test #2" in
CVS sources

  % grep -nr 'printf\W\+double' packages/language/c/
  packages/language/c/libc/stdio/current/tests/sprintf2.c:169:                 
     "snprintf double test #1");
  packages/language/c/libc/stdio/current/tests/sprintf2.c:172:                 
     "snprintf double test #1 return code");

Could you clarify, please, what the test does not pass?

Sergei

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #4 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-08 08:30:42 BST ---
(In reply to comment #3)
oops, sorry... Now I see, actually I applied the my own patch from
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001628
a while ago, it contained a new test case...

     // Check 7
     ret = snprintf(y, 20, "bit of that: %.4g double", 1.0012);
     my_strcpy(z, "bit of that: 1.001 ");
     CYG_TEST_PASS_FAIL(my_strcmp(y,z) == 0,
                        "snprintf double test #2");

     CYG_TEST_PASS_FAIL(ret == my_strlen(z),
                        "snprintf double test #2 return code");

when that did fail after applying your patch, I was surprised.

Well, there is more trouble with sprintf....

There is another bug #20804, about the %e format, that is also still
not fixed.

When I tried to understand, why that happens, I found a buffer overflow
in the statement: printf("%.15f", DBL_MAX);

There is a buffer in vfnprintf() which is 2 characters too small.
Maybe there should be a patch for all of the known issues.
What do you think?

Bernd.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #5 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2012-08-08 17:58:35 BST ---
(In reply to comment #4)
> (In reply to comment #3)
> oops, sorry... Now I see, actually I applied the my own patch from
> http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001628

[snip]

Thanks. Clear.

> Well, there is more trouble with sprintf....
> 
> There is another bug #20804, about the %e format, that is also still
> not fixed.
> 
> When I tried to understand, why that happens, I found a buffer
> overflow in the statement: printf("%.15f", DBL_MAX);
> 
> There is a buffer in vfnprintf() which is 2 characters too small.

There is another case.  Today I investigated in bug #20804.  The reason
is

686             /* trailing f.p. zeroes */
687             PAD(fpprec, zeroes);
688             ret += fpprec;

It's okay for ("%.18f\n", 3.14e-11)

   0.000000000031400000

But the padding/zeroing will be wrong for %e, %E, when requested prec >
MAXPREC. Well, I ever made a fix :-) But, then I Googled and found this
report

  http://www.cygwin.com/ml/ecos-discuss/2001-05/msg00065.html

Well, it looks like my fix (Suzuki did talk about the same point which I
found in GDB), but my workaround was

        if (prec > MAXFRACT) {
                if ((ch == 'f' && ch == 'F') || (flags&ALT)) {
                        fpprec = prec - MAXFRACT;
                        prec = MAXFRACT;
                }
        } else if (prec == -1)

I was suprised how many projects use the same "printf" sources! But I've
seen no fixes in this place there.  I need more time to look around. 

> Maybe there should be a patch for all of the known issues.  What do
> you think?

No matter.  I think we have to rid all the issues. Thank you for your
reports.

Sergei

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #6 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-09 08:49:57 BST ---
(In reply to comment #5)
> It's okay for ("%.18f\n", 3.14e-11)
>    0.000000000031400000

but not quite OK for these:
("%.18f\n", 3.1415926E-11)
eCos: "0.000000000031400000"
glib: "0.000000000031415926"

("%.18f\n", DBL_MAX*2)
eCos: "inf000"
glib: "inf"


> But the padding/zeroing will be wrong for %e, %E, when requested prec >
> MAXPREC.

but also for %g: this does work like %e, when the value is >=10^prec or <=10^-4

> Well, it looks like my fix (Suzuki did talk about the same point which I
> found in GDB), but my workaround was
>         if (prec > MAXFRACT) {
>                 if ((ch == 'f' && ch == 'F') || (flags&ALT)) {
>                         fpprec = prec - MAXFRACT;
>                         prec = MAXFRACT;
>                 }
>         } else if (prec == -1)

ok, but with this patch there will be a crash in printf("%.999e", x)

limiting prec MAXFRACT helps to avoid the buffer overrun in "cvt"
however with DBL_MAX the buffer size BUF 2 characters too small as I said.

I tried to solve it this way:

diff -Nur
ecos-cvs-120723/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx
ecos/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx
---
ecos-cvs-120723/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx
    2009-08-20 18:09:18.000000000 +0200
+++ ecos/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx      
2012-08-07 10:16:48.809576300 +0200
@@ -107,7 +107,7 @@
 # define MAXFRACT  DBL_DIG
 # define MAXEXP    DBL_MAX_10_EXP

-# define BUF             (MAXEXP+MAXFRACT+1)     /* + decimal point */
+# define BUF             (MAXEXP+MAXFRACT+3)     /* + decimal point + rounding
*/
 # define DEFPREC         6

 static int
@@ -420,7 +420,7 @@
                          * zeroes later, so buffer size stays rational.
                          */
                         if (prec > MAXFRACT) {
-                                if ((ch != 'g' && ch != 'G') || (flags&ALT))
+                                if (ch == 'f' || ch == 'F')
                                         fpprec = prec - MAXFRACT;
                                 prec = MAXFRACT;
                         } else if (prec == -1)


This way there are no buffer overruns, and the added zeros are at least
never in the exponent. That would at least be a interim solution...

But I start to think that the "cvt" function will need a complete re-write
for strict conformance. And another point would be, that when you look at
the vfnprintf function in the assembler (ARM9, eCosCentric GNU tools 4.3.2-sw)

vfnprintf:stmdb   r13!,{r4-r11,r14}
          sub     r13,r13,#0x30C

This function needs 816 bytes on the stack,
even if you do not use any %f formats!

Maybe reducing this number could be worth the effort.

Bernd.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #7 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2012-08-09 09:52:06 BST ---
(In reply to comment #6)
> (In reply to comment #5)
> > It's okay for ("%.18f\n", 3.14e-11)
> >    0.000000000031400000
> 
> but not quite OK for these:
> ("%.18f\n", 3.1415926E-11)
> eCos: "0.000000000031400000"
> glib: "0.000000000031415926"
> 
> ("%.18f\n", DBL_MAX*2)
> eCos: "inf000"
> glib: "inf"
> 
> 
> > But the padding/zeroing will be wrong for %e, %E, when requested prec >
> > MAXPREC.
> 
> but also for %g: this does work like %e, when the value is >=10^prec or <=10^-4
> 
> > Well, it looks like my fix (Suzuki did talk about the same point which I
> > found in GDB), but my workaround was
> >         if (prec > MAXFRACT) {
> >                 if ((ch == 'f' && ch == 'F') || (flags&ALT)) {
> >                         fpprec = prec - MAXFRACT;
> >                         prec = MAXFRACT;
> >                 }
> >         } else if (prec == -1)
> 
> ok, but with this patch there will be a crash in printf("%.999e", x)

You're right. I had not tested it enough.

> limiting prec MAXFRACT helps to avoid the buffer overrun in "cvt"
> however with DBL_MAX the buffer size BUF 2 characters too small as I said.
> 
> I tried to solve it this way:
> 
> diff -Nur
> ecos-cvs-120723/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx
> ecos/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx
> ---
> ecos-cvs-120723/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx
>     2009-08-20 18:09:18.000000000 +0200
> +++ ecos/packages/language/c/libc/stdio/current/src/output/vfnprintf.cxx      
> 2012-08-07 10:16:48.809576300 +0200
> @@ -107,7 +107,7 @@
>  # define MAXFRACT  DBL_DIG
>  # define MAXEXP    DBL_MAX_10_EXP
> 
> -# define BUF             (MAXEXP+MAXFRACT+1)     /* + decimal point */
> +# define BUF             (MAXEXP+MAXFRACT+3)     /* + decimal point + rounding
> */
>  # define DEFPREC         6
> 
>  static int
> @@ -420,7 +420,7 @@
>                           * zeroes later, so buffer size stays rational.
>                           */
>                          if (prec > MAXFRACT) {
> -                                if ((ch != 'g' && ch != 'G') || (flags&ALT))
> +                                if (ch == 'f' || ch == 'F')
>                                          fpprec = prec - MAXFRACT;
>                                  prec = MAXFRACT;
>                          } else if (prec == -1)
> 
> 
> This way there are no buffer overruns, and the added zeros are at least
> never in the exponent. That would at least be a interim solution...

Yes, as for me it's better than nothing. BTW, tonight I found a few points on
+3. E.g.

http://sources.redhat.com/ml/newlib/2003/msg00610.html

http://svn.deepdarc.com/code/contiki/trunk/cpu/stm32w108/hal/micro/cortexm3/e_stdio/src/small_vfsscanf.c

> But I start to think that the "cvt" function will need a complete re-write
> for strict conformance. And another point would be, that when you look at
> the vfnprintf function in the assembler (ARM9, eCosCentric GNU tools 4.3.2-sw)
> 
> vfnprintf:stmdb   r13!,{r4-r11,r14}
>           sub     r13,r13,#0x30C
> 
> This function needs 816 bytes on the stack,
> even if you do not use any %f formats!
> 
> Maybe reducing this number could be worth the effort.

Even if CYGSEM_LIBC_STDIO_PRINTF_FLOATING_POINT is not defined?                 

Bernd, could you, please, prepare the patch on 0-padding with ChangeLog         
entry?  It seems to me the patch can be submitted for bug #20804               
record.

Thank you,
Sergei

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #8 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-09 11:00:15 BST ---
(In reply to comment #7)
Sergei,

> > This function needs 816 bytes on the stack,
> > even if you do not use any %f formats!
> > 
> > Maybe reducing this number could be worth the effort.
> Even if CYGSEM_LIBC_STDIO_PRINTF_FLOATING_POINT is not defined?                 

hmm, thats funny.....

Without that define it is only 152 bytes.
That is 664 bytes difference.
BUT #define BUF changes only from 40 to 326.

Incredible, but I think I can reduce this easily:
cvt(), round(), and exponent() are all inlined.
However, char expbuf[MAXEXP] is 308 bytes!!!

but the largest possible exponent, "308", needs 3 only bytes.
Good you asked about that. Thanks!

This will reduce the stack use by 305 bytes.


> Bernd, could you, please, prepare the patch on 0-padding with ChangeLog         
> entry?  It seems to me the patch can be submitted for bug #20804               
> record.
> Thank you,
> Sergei

Ok, I am going to fix that stack issue too, and if you do not mind,
I'd also like to fix the %g issue [Bug #1001628] and submit a cumulative
patch at [Bug #20804].

Thanks,
Bernd.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #9 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2012-08-09 11:52:14 BST ---
(In reply to comment #8)

> Ok, I am going to fix that stack issue too, and if you do not mind,
> I'd also like to fix the %g issue [Bug #1001628] and submit a cumulative
> patch at [Bug #20804].

Feel free. It's okay for me.

Regarding Bug #1001628, do you resist on new check for `sprintf2.c?' Could you
reject it from the final patch? Why I ask, IMHO, your patches are small enough
to commit them without a copyright assignment, otherwise the copyright
assignment is required for any significant changes to the core eCos packages:

  http://ecos.sourceware.org/assign.html

Thank you,
Sergei

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #10 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-09 12:03:55 BST ---
(In reply to comment #9)
> Feel free. It's okay for me.
> Regarding Bug #1001628, do you resist on new check for `sprintf2.c?' Could you
> reject it from the final patch? Why I ask, IMHO, your patches are small enough
> to commit them without a copyright assignment, otherwise the copyright
> assignment is required for any significant changes to the core eCos packages:
>   http://ecos.sourceware.org/assign.html
> Thank you,
> Sergei

Ok. Done.

Of course I did not touch the snprintf2.c test file, as it is obvious,
that your patch would break otherwise..

Thanks
Bernd.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |john@dallaway.org.uk

--- Comment #11 from John Dallaway <john@dallaway.org.uk> ---
Sergei, is there any reason why we should not check-in your patch for the
original null terminator issue reported in this bug?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

--- Comment #12 from Sergei Gavrikov <sergei.gavrikov@gmail.com> ---
(In reply to comment #11)
> Sergei, is there any reason why we should not check-in your patch for the
> original null terminator issue reported in this bug?

Ah! Thanks for reminder, I'll do it tonight.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001490] C99 snprintf() does not include terminated null in truncated strings

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001490

Sergei Gavrikov <sergei.gavrikov@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |CURRENTRELEASE

--- Comment #13 from Sergei Gavrikov <sergei.gavrikov@gmail.com> ---
Checked-in.

-- 
You are receiving this mail because:
You are on the CC list for the bug.