After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 756382 - snprintf used on Windows with VS2015 doesn't support %n
snprintf used on Windows with VS2015 doesn't support %n
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: win32
2.46.x
Other Windows
: Normal normal
: ---
Assigned To: gtk-win32 maintainers
gtk-win32 maintainers
Depends on:
Blocks:
 
 
Reported: 2015-10-11 09:02 UTC by Arnav Singh
Modified: 2015-10-15 15:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gnulib: %n is not supported on old glibc or on native win32 (2.40 KB, patch)
2015-10-12 07:24 UTC, Ignacio Casal Quinteiro (nacho)
committed Details | Review
Update gnulib (320.99 KB, patch)
2015-10-15 11:57 UTC, Ignacio Casal Quinteiro (nacho)
committed Details | Review
gnulib: forgot some changes from HAVE_LONG_LONG_INT to HAVE_LONG_LONG (4.06 KB, patch)
2015-10-15 15:42 UTC, Ignacio Casal Quinteiro (nacho)
committed Details | Review

Description Arnav Singh 2015-10-11 09:02:17 UTC
As reported in https://mail.gnome.org/archives/gtk-devel-list/2015-October/msg00007.html the vasnprintf() function from gnulib uses the CRT's snprintf function with a %n flag, which causes the program to abort when using the VS 2015 CRT. Earlier versions of VS CRT did not have snprintf at all so did not have a problem. VS2015 does provide it but aborts the program because it considers %n an invalid flag.

This was fixed in upstream gnulib by not appending %n to the flags under Windows even if snprintf is available - http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blobdiff;f=lib/vasnprintf.c;h=968835a1e7d5a704ae5a330ecda091b78851f73c;hp=8377d31eb591687186815e1de5cf973aac465120;hb=ba739e5686c1488280341451c3eb01a8f7b0aaa1;hpb=8c1ff97529b4f5b6462f1be302cc526ccb1960a1

As suggested by LRN in the ML thread, glib could fix this by updating its copy of gnulib. I've worked around it for now by porting just that one line and it seems to be fine.
Comment 1 Ignacio Casal Quinteiro (nacho) 2015-10-12 07:24:48 UTC
Created attachment 313104 [details] [review]
gnulib: %n is not supported on old glibc or on native win32

On old glibc even if %n is supported we may get a crash, so it is
better to skip it while on native win32 %n will abort the program.
These changes were took from upstream.
Comment 2 Ignacio Casal Quinteiro (nacho) 2015-10-12 07:27:36 UTC
So basically here I just took the code from upstream. I think it is fine but I have no way to test it on linux. I guess we have to wait for Matthias for a review on this.
Comment 3 Fan, Chun-wei 2015-10-12 09:47:25 UTC
Hi Nacho,

(In reply to Ignacio Casal Quinteiro (nacho) from comment #2)
> So basically here I just took the code from upstream. I think it is fine but
> I have no way to test it on linux. I guess we have to wait for Matthias for
> a review on this.

I'd agree with you.

With blessings, thank you!
Comment 4 Ignacio Casal Quinteiro (nacho) 2015-10-14 12:23:46 UTC
I went ahead and I updated gnulib, It can be found in wip/nacho/gnulib.
Comment 5 Fan, Chun-wei 2015-10-14 14:52:54 UTC
Hi Nacho,

I think it is fine to merge the gnulib updates in wip/nacho/gnulib into master (and glib-2-46 as well).  I have tested it by building on Visual Studio 2008~2015 and ran the tests in glib\tests, which the related items passed (in particular the test-printf test program--it also ensures that the snprintf() implementation for Visual Studio 2015 is indeed good enough).

With blessings, thank you!
Comment 6 Allison Karlitskaya (desrt) 2015-10-15 11:45:43 UTC
Review of attachment 313104 [details] [review]:

Please put this on glib-2-46.

Meanwhile, improve the commit message on the branch to mention that it has been tested on Windows and also mention the changes you introduced vs. the upstream version.  Attach that here and I will probably ACK it for master, but definitely not for backporting.
Comment 7 Ignacio Casal Quinteiro (nacho) 2015-10-15 11:49:49 UTC
Comment on attachment 313104 [details] [review]
gnulib: %n is not supported on old glibc or on native win32

Attachment 313104 [details] pushed as 94688bc - gnulib: %n is not supported on old glibc or on native win32
Comment 8 Ignacio Casal Quinteiro (nacho) 2015-10-15 11:57:06 UTC
Created attachment 313361 [details] [review]
Update gnulib

It updates it to the version c5d07ce91a8ad51591154450442fa4376441fdfa
As a difference with upstream we need to ensure:
 * Include "g-gnulib.h" so the methods get the gnulib namespace.
 * xsize.h uses G_MAXSIZE instead of SIZE_MAX and the methods are
   marked as static inline.
 * Some defines are named different from the ones in glib i.e
   HAVE_LONG_LONG_INT is HAVE_LONG_LONG

All the unit tests pass properly with and without --enable-included-printf.
It has also been tested on Windows.
Comment 9 Allison Karlitskaya (desrt) 2015-10-15 12:53:02 UTC
Review of attachment 313361 [details] [review]:

Thanks for being so thorough about this.

Please commit, only to master.
Comment 10 Ignacio Casal Quinteiro (nacho) 2015-10-15 12:55:11 UTC
Attachment 313361 [details] pushed as 212e423 - Update gnulib
Comment 11 Arnav Singh 2015-10-15 15:33:47 UTC
glib/gnulib/printf-parse.c contains three instances of HAVE_LONG_LONG_INT
Comment 12 Ignacio Casal Quinteiro (nacho) 2015-10-15 15:42:10 UTC
Created attachment 313384 [details] [review]
gnulib: forgot some changes from HAVE_LONG_LONG_INT to HAVE_LONG_LONG
Comment 13 Ignacio Casal Quinteiro (nacho) 2015-10-15 15:43:14 UTC
Comment on attachment 313384 [details] [review]
gnulib: forgot some changes from HAVE_LONG_LONG_INT to HAVE_LONG_LONG

Attachment 313384 [details] pushed as 0b84596 - gnulib: forgot some changes from HAVE_LONG_LONG_INT to HAVE_LONG_LONG
Comment 14 Ignacio Casal Quinteiro (nacho) 2015-10-15 15:44:02 UTC
(In reply to Arnav Singh from comment #11)
> glib/gnulib/printf-parse.c contains three instances of HAVE_LONG_LONG_INT

Thanks, I recall changing it too, but at some point I had to overwrite and start from scratch so I guess I missed it on the second round. I pushed the new patch. Let me know if you still have issues.