Re: [Chicken-hackers] [PATCH] Fix #566 and simplify/improve flonum printing code
On Sun, Dec 01, 2013 at 11:31:31AM -0500, John Cowan wrote: > Peter Bex scripsit: > > > However, I've tested all three Windows builds, and they all behave > > equally well (or better) with s[n]printf() instead of gcvt(). We no > > longer have a MSVC build, so the original problem probably was there, > > As of Visual C++ 2005, Microsoft deprecated all the functions in the C > Runtime Library that aren't part of ISO C. So snprintf, which is part > of Posix but not ISO C, appears as _snprintf. This is a general problem > which we'll have to solve if we ever want a VC++ build. In some cases, > like gcvt, the original function survives in deprecated form, but _gcvt > is recommended. This is all in the name of avoiding namespace pollution, > a silly concern on Windows with its monstrous file. We'll fix that when we get to it. I think there may be other calls like it which don't work as-is. We could fix it in chicken.h by aliasing it to C_snprintf or do it some other way. Cheers, Peter -- http://www.more-magic.net ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Fix #566 and simplify/improve flonum printing code
Peter Bex scripsit: > However, I've tested all three Windows builds, and they all behave > equally well (or better) with s[n]printf() instead of gcvt(). We no > longer have a MSVC build, so the original problem probably was there, As of Visual C++ 2005, Microsoft deprecated all the functions in the C Runtime Library that aren't part of ISO C. So snprintf, which is part of Posix but not ISO C, appears as _snprintf. This is a general problem which we'll have to solve if we ever want a VC++ build. In some cases, like gcvt, the original function survives in deprecated form, but _gcvt is recommended. This is all in the name of avoiding namespace pollution, a silly concern on Windows with its monstrous file. In both GNU libc and newlib, gcvt just calls sprintf. > I've also taken the opportunity to convert sprintf into a checked > snprintf. I'm not 100% sure but I don't think this requires a CVE > since you can't easily (at all?) cause over 4096 flonum digits to > get printed, It would be nonsense to do so anyway: #;1> (parameterize ((flonum-print-precision 4095)) (print 1.1)) 1.100088817841970012523233890533447265625 All those low-order digits represent the triumph of precision over accuracy. I suppose some day we could integrate MPFR into the numbers egg, though. -- John Cowanco...@ccil.orghttp://ccil.org/~cowan Rather than making ill-conceived suggestions for improvement based on uninformed guesses about established conventions in a field of study with which familiarity is limited, it is sometimes better to stick to merely observing the usage and listening to the explanations offered, inserting only questions as needed to fill in gaps in understanding. --Peter Constable ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Fix #566 and simplify/improve flonum printing code
Hi Peter, Peter Bex writes: > > In order to fix #566, I decided it's much easier to rip out the > HAVE_GCVT definition in Cygwin. After testing on several platforms, > it turns out that gcvt() is really not required, and it's deprecated > by POSIX as well. So we should probably stop using it anyway; this > also simplifies testing as it doesn't needlessly use different library > functions on different platforms. > > I tried to figure out why HAVE_GCVT was introduced in the first place, > but it goes all the way back to the very first commit in the NonGNU CVS > even. Felix mentioned that he seemed to recall that it had something > to do with making behaviour of the various Windows builds consistent > among eachother. > > However, I've tested all three Windows builds, and they all behave > equally well (or better) with s[n]printf() instead of gcvt(). We no > longer have a MSVC build, so the original problem probably was there, > or it was in an older version of Cygwin/MingW which has been fixed in > the meanwhile (it's been over 10 years, so a thing or two will have > changed there, as well!). If we ever regain a MSVC build, we can > look at restoring this code, but in the meanwhile I prefer the > simplicity of this solution. I was even able to get rid of the > MINGW hack just below the changed code! wow, thanks going through all that trouble! Shedding some light on dusty corners like that is definitely a good idea to keep the code clean and to improve it, too. > I've also taken the opportunity to convert sprintf into a checked > snprintf. I'm not 100% sure but I don't think this requires a CVE > since you can't easily (at all?) cause over 4096 flonum digits to > get printed, and flonum-print-precision is rarely, if ever > user-controlled. Feel free to request a CVE if you disagree. I'm not sure about this either. I've pushed the patch though as it looks OK to me and all tests pass, too. I also grepped for "gcvt" afterwards and couldn't find traces of it anymore. Good riddance! Moritz ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
[Chicken-hackers] [PATCH] Fix #566 and simplify/improve flonum printing code
Hi all, In order to fix #566, I decided it's much easier to rip out the HAVE_GCVT definition in Cygwin. After testing on several platforms, it turns out that gcvt() is really not required, and it's deprecated by POSIX as well. So we should probably stop using it anyway; this also simplifies testing as it doesn't needlessly use different library functions on different platforms. I tried to figure out why HAVE_GCVT was introduced in the first place, but it goes all the way back to the very first commit in the NonGNU CVS even. Felix mentioned that he seemed to recall that it had something to do with making behaviour of the various Windows builds consistent among eachother. However, I've tested all three Windows builds, and they all behave equally well (or better) with s[n]printf() instead of gcvt(). We no longer have a MSVC build, so the original problem probably was there, or it was in an older version of Cygwin/MingW which has been fixed in the meanwhile (it's been over 10 years, so a thing or two will have changed there, as well!). If we ever regain a MSVC build, we can look at restoring this code, but in the meanwhile I prefer the simplicity of this solution. I was even able to get rid of the MINGW hack just below the changed code! I've also taken the opportunity to convert sprintf into a checked snprintf. I'm not 100% sure but I don't think this requires a CVE since you can't easily (at all?) cause over 4096 flonum digits to get printed, and flonum-print-precision is rarely, if ever user-controlled. Feel free to request a CVE if you disagree. Cheers, Peter -- http://www.more-magic.net >From beeaaf02c5e675fee031ed5eb856d6c391636e85 Mon Sep 17 00:00:00 2001 From: Peter Bex Date: Mon, 18 Nov 2013 22:06:13 +0100 Subject: [PATCH] Remove HAVE_GCVT check and definition. Use snprintf on all platforms. gcvt() is deprecated in POSIX, and it produces problematic output in at least cygwin. For example, -0.0 is printed as "0.0" and more fun things. This fix adds a regression test for the negative zero case. Replace sprintf usage with snprintf just in case someone decides to use insane values for flonum_print_precision. --- Makefile.aix |1 - Makefile.cross-linux-mingw|1 - Makefile.cygwin |1 - Makefile.haiku|1 - Makefile.hurd |1 - Makefile.linux|1 - Makefile.mingw|1 - Makefile.mingw-msys |1 - Makefile.solaris |1 - chicken.h |1 - runtime.c | 17 +++-- tests/numbers-string-conversion-tests.scm |4 +++- 12 files changed, 6 insertions(+), 25 deletions(-) diff --git a/Makefile.aix b/Makefile.aix index d453499..8bde946 100644 --- a/Makefile.aix +++ b/Makefile.aix @@ -97,7 +97,6 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_ALLOCA_H 1" >>$@ echo "#define HAVE_GRP_H 1" >>$@ echo "#define HAVE_ERRNO_H 1" >>$@ - echo "#define HAVE_GCVT 1" >>$@ echo "#define HAVE_SYSEXITS_H 1" >>$@ echo "#define C_STACK_GROWS_DOWNWARD 1" >>$@ ifdef GCHOOKS diff --git a/Makefile.cross-linux-mingw b/Makefile.cross-linux-mingw index c18dd18..e1778ca 100644 --- a/Makefile.cross-linux-mingw +++ b/Makefile.cross-linux-mingw @@ -116,7 +116,6 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_ALLOCA_H 1" >>$@ echo "#define HAVE_DIRECT_H 1" >>$@ echo "#define HAVE_ERRNO_H 1" >>$@ - echo "#define HAVE_GCVT 1" >>$@ echo "#define HAVE_LOADLIBRARY 1" >>$@ echo "#define HAVE_GETPROCADDRESS 1" >>$@ echo "#define HAVE_WINSOCK2_H 1" >>$@ diff --git a/Makefile.cygwin b/Makefile.cygwin index b4122ef..1518a16 100644 --- a/Makefile.cygwin +++ b/Makefile.cygwin @@ -113,7 +113,6 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_ALLOCA_H 1" >>$@ echo "#define HAVE_GRP_H 1" >>$@ echo "#define HAVE_ERRNO_H 1" >>$@ - echo "#define HAVE_GCVT 1" >>$@ echo "#define HAVE_SYSEXITS_H 1" >>$@ echo "#define HAVE_DLFCN_H 1" >>$@ echo "#define C_STACK_GROWS_DOWNWARD 1" >>$@ diff --git a/Makefile.haiku b/Makefile.haiku index c6e33ab..63da7c0 100644 --- a/Makefile.haiku +++ b/Makefile.haiku @@ -91,7 +91,6 @@ chicken-config.h: chicken-defaults.h echo "#define HAVE_ALLOCA_H 1" >>$@ echo "#define HAVE_GRP_H 1" >>$@ echo "#define HAVE_ERRNO_H 1" >>$@ - echo "#define HAVE_GCVT 1" >>$@ echo "#define HAVE_MEMMOVE 1" >>$@ echo "#define C_STACK_GROWS_DOWNWARD 1" >>$@ echo "#define SIGIO 0" >>$@ diff --git a/Makefile.hurd b/Makefile.hurd index 74c8c00..1a64158 100644 --- a/Makefile.hurd +++ b/Makefile.hurd @@ -92,7 +92,6 @@ chicken-config.h: chicken-defaults.h echo "#define
