MantisBT - OCaml
View Issue Details
0006075OCamlruntime system and C interfacepublic2013-07-14 21:112015-12-11 19:26
jfc 
xleroy 
lowminoralways
closedopen 
amd64OpenBSD5.3
4.00.1 
4.01.1+dev4.02.0+dev 
0006075: Compile warnings on OpenBSD
OpenBSD prints warnings when unsafe library functions like strcpy are used. These functions are used internally in ocaml, so linking any native code program will cause warnings like

/usr/local/lib/ocaml/libasmrun.a(sys.o)(.text+0x7f7): In function `caml_sys_open':
: warning: strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/ocaml/libasmrun.a(unix.o)(.text+0x265): In function `caml_search_in_path':
: warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/ocaml/libasmrun.a(ints.o)(.text+0x622): In function `caml_nativeint_format':
: warning: sprintf() is often misused, please use snprintf()


The functions are used safely, but the linker has no way to know they are used safely.

Fixing this for modern Unix systems is simple. However, Windows does not have snprintf and strlcpy. The strcpy and strcat warnings can be portably avoided by using strlen+memcpy. Avoiding the warning for sprintf would require #ifdef to test which C runtime library is available.
patch
c bsd-snprintf.c (20,195) 2013-07-16 00:35
https://caml.inria.fr/mantis/file_download.php?file_id=962&type=bug
c sprintf.c (1,309) 2013-07-29 17:31
https://caml.inria.fr/mantis/file_download.php?file_id=986&type=bug
? sprintf-diff (442) 2013-08-01 23:29
https://caml.inria.fr/mantis/file_download.php?file_id=1005&type=bug
c sprintf-2.c (1,489) 2014-02-05 15:56
https://caml.inria.fr/mantis/file_download.php?file_id=1133&type=bug
Issue History
2013-07-14 21:11jfcNew Issue
2013-07-15 16:01xleroyNote Added: 0009778
2013-07-15 16:01xleroyStatusnew => acknowledged
2013-07-16 00:34avsmNote Added: 0009780
2013-07-16 00:35avsmFile Added: bsd-snprintf.c
2013-07-29 17:31xleroyFile Added: sprintf.c
2013-07-29 17:31xleroyNote Added: 0009990
2013-08-01 23:29jfcFile Added: sprintf-diff
2013-08-01 23:35jfcNote Added: 0010073
2013-08-19 16:56doligezTarget Version => 4.01.1+dev
2014-01-17 16:05doligezNote Added: 0010806
2014-01-17 16:06doligezTag Attached: patch
2014-02-05 15:55oandrieuNote Added: 0010883
2014-02-05 15:56oandrieuFile Added: sprintf-2.c
2014-02-05 17:09xleroyNote Added: 0010884
2014-04-15 19:16xleroyNote Added: 0011285
2014-04-15 19:16xleroyAssigned To => xleroy
2014-04-15 19:16xleroyStatusacknowledged => resolved
2014-04-15 19:16xleroyFixed in Version => 4.02.0+dev
2015-12-11 19:26xleroyStatusresolved => closed
2017-02-23 16:43doligezCategoryOCaml runtime system => runtime system
2017-03-03 17:45doligezCategoryruntime system => runtime system and C interface

Notes
(0009778)
xleroy   
2013-07-15 16:01   
There is something unpleasant in having to rewrite working code (I think) just to silence linker warnings. Moreover, strlcpy and strlcat are BSD-only functions: they are not part of any standard (C, POSIX, SUS, etc) that I know of, and are not even available in GlibC. This adds insult to injury.

In more details: I agree the uses of strcpy and strcat mentioned in the PR can easily be rewritten without. I also agree that snprintf would make the number formatting functions in ints.c and floats.c not only safer, but also simpler. (We could let snprintf tell us how many bytes of output are really needed, instead of guessing beforehand.)

So, Windows is, as usual, the limiting factor. Note that recent (how recent?) MS CRT libraries have _snprintf, which is close except that it doesn't compute the number of characters to be written...
(0009780)
avsm   
2013-07-16 00:34   
One option is to bundle a copy of bsd-snprintf.c from the openssh-portable distribution, and use the local copy if the function isn't detected in a configure script. Is this even an option for OCaml, or is the licensing a problem?

Without that, all portability paths seem to require keeping the format length calculation code around behind an #ifdef, thanks to Windows' remarkable choice of behaviour if the buffer is too small in _sprintf.

Incidentally, the linker warning was introduced after sweeping most of the OpenBSD source tree clean of strcpy/strcat use. The recommendation to use strlcpy is a local one only -- we obviously can't control whether glibc chooses to adopt those interfaces or not. A quick glance around the uses in OCaml do look like they always know the string length just before the strcpy, and can be easily rewritten using a more explicit memmove.

I can introduce a local patch to the OpenBSD port for snprintf very easily (or even use asprintf as a slower but simpler change), but would prefer that the strcpy/strcat use is fixed upstream before adding a port-patch.
(0009990)
xleroy   
2013-07-29 17:31   
More investigation suggests that MS CRT provides not just non-C99-compliant _snprintf and _vsnprintf functions, but also a _vscprintf function that computes the length of the output without generating it. Stitching the two together, we may have a solution.

Attached is a proof-of-concept C implementation of a caml_alloc_sprintf() function that works just like sprintf(), but allocates its result in the Caml heap and returns it. Untested yet; code review and comments most welcome. This new function would be a very welcome replacement to the hacks we currently do around sprintf for formatting numbers.
(0010073)
jfc   
2013-08-01 23:35   
It is necessary to call va_end and va_start before calling vsnprintf a second time. Diff uploaded.

I may be away from my OpenBSD system for a few weeks. I will do more testing when I return.
(0010806)
doligez   
2014-01-17 16:05   
> It is necessary to call va_end and va_start before calling vsnprintf a second time. Diff uploaded.

I don't know anything about varargs, but this is probably also the case for the _vscprintf and second _vsnprintf in the Windows branch.
(0010883)
oandrieu   
2014-02-05 15:55   
FYI, mingw does provide C99-compliant snprintf & vsnprintf. So both versions should work for mingw.

I attached a sprintf.c version with va_start/va_end wrapping all *vprintf calls. calls added.
(0010884)
xleroy   
2014-02-05 17:09   
Thanks, Olivier, for the info. I have a first cut of a patch that replaces strcpy, sprintf et al with safer code. Just need to clean it and have it reviewed. But after seeing http://natashenka.ca/posters/ [^] I agree this is worth fixing by the next major release :-)
(0011285)
xleroy   
2014-04-15 19:16   
Tentative fix in commit 14607 on trunk. Works fine under Linux and BSD but needs testing under Windows. Should normally go in OCaml 4.02.