Wow, this is a lot to go through, but I really do appreciate it.
It may take me a bit to sort through these, Could send them to me as discrete
patches (e.g. like a git send-email patch series)? That would make them easier
to sort through and apply individually?
On Jul 16, 2014, at 5:02 PM, Jonas 'Sortie' Termansen <sor...@maxsi.org> wrote:
> Hi,
>
> I ported libressl to my custom hobby OS and it has been a pleasant
> experience. Nonetheless, I did run into some minor portability problems
> that I wish to share:
What sort of OS is this?
> * apps/Makefile.am.tpl links libcrypto and libssl in the wrong order.
> The libssl library depends on libcrypto and libcrypto doesn't depend
> on libssl. Linking the openssl program as -lcrypto -lssl breaks on
> static linking if the main program didn't pull in parts of libcrypto
> that libssl needs.
Right, sounds good.
> * The POSIX <sys/select.h> header is not used to get the select
> function. While <sys/time.h> is required to also provide it (though
> that seems transitional to me), there is no requirement <sys/time.h>
> also provide the FD_* macros needed to actually use select. This
> inclusion needs to be added in apps/ocsp.c, apps/s_client.c,
> apps/s_server.c and apps/s_time.c.
> * ioctl(FIONBIO) is used in a few files to make sockets non-blocking
> rather than using fcntl to set the the standard O_NONBLOCK bit. The
> files apps/s_client.c and apps/s_server.c should use BIO_socket_nbio()
> instead of directly using BIO_socket_ioctl(). The bio/b_sock.c file
> should implement BIO_shock_nbio() using fcntl, F_GETFL/F_SETFL and
> O_NONBLOCK.
> * include/machine/endian.h wraps the <endian.h> header if Linux and the
> <machine/endian.h> header if non-Linux. However, it may be the case
> that a non-Linux system (such as mine) has <endian.h> and not
> <machine/endian.h>. The POSIX committee has accepted <endian.h> into
> the next revision of POSIX. guenther recently implemented this draft
> in OpenBSD and the header is already present on many modern Unix
> systems. It would be better if the compatibility layer instead
> implement <endian.h> rather than <machine/endian.h>.
There were some recent updates to OpenBSD as well to become more POSIX
compliant in this area, so <machine/endian.h> will soon move.
> * The build system defaults --program-transform-name to the host triplet
> when cross-compiling. It shouldn't do that as the library doesn't have
> a target and is not a cross-compiler (as far as I know). It certainly
> doesn't make sense to do when the library itself is being
> cross-compiled. This also caused unexpected trouble during the man
> page installation as the real man pages got installed under the
> transformed name, but the hard links to the man pages wasn't
> transformed and broke the build.
> * The non-standard err(3) family is used a few places. In the main
> library it is only used in the apps/openssl.c file, while it is used a
> few times in the regression tests. The convenience of using these
> functions is not worth the portability concerns when the calling code
> can easily use fprintf and exit instead.
We had half started some a compat err.h at the hackathon, but fair enough.
> * The configure.ac file doesn't check if explicit_bzero is provided by
> the operating system but unconditionally uses the internal one.
There are some OS-provided explicit_bzero implementations that use a function
pointer to beat the compiler optimization. This pointer can possibly be
overwritten with a different function.
But generally, we can probably make that happen.
> I also have a list of minor portability issues I don't mind patching
> locally and other stray suggestions:
>
> * crypto/compat/issetugid_linux.c is used on non-Linux platforms. This
> fail on including glibc internal headers which is hardly elegant.
> Perhaps another file should be used for unknown operating systems that
> assumes the worst and throws a #warning? It could also be handled like
> getentropy where it fails with linker errors.
That could work. If it is your own OS, you could perhaps implement issetugid
and bypass the compatibility layer entirely.
> * ssl/d1_lib.c includes the non-standard header <sys/param.h>. It worked
> when I removed the inclusion, so it seems redundant.
> * The non-standard u_char and u_int types are used a few places in the
> random compatibility code. It would work just as well if those were
> replaced with explicit unsigned char and unsigned int types.
Yah, I have a local patch I need to push that gets rid of these.
Old habits are hard to break.
> * Consider using _DEFAULT_SOURCE or _ALL_SOURCE as feature macros on
> unknown platforms.
> * My strerror() (when certain feature macros are defined) return a const
> char* as it would be an error to modify the storage. This triggered on
> a strerror() invocation in crypto/err/err.c where it stores the result
> in a char * rather than a safe const char *.
This didn’t cause a const-ripple through the code? Color me surprised!
> * There should be an explicit counterpart of memset called
> explicit_memset. My libc has such a function, but no explicit_bzero as
> I have no regular bzero. If an explicit_memset is added (or detected
> in the host system), it could be used in the compatibility
> implementation of explicit_bzero (which is currently implemented by
> calling memset). I would much prefer if the library uses a safe
> zeroing function from the standard library (rather than its own) as
> that would give the standard library a chance to be better: I'm quite
> interested in using new compiler extensions to robustly ensure memory
> is cleared even in the face of link-time optimization and sufficiently
> advanced compilers. I don't have absolute confidence that the weak
> symbol trick used in explicit_bzero.c is good enough.
Fair enough, though I believe we have a pretty good test that verifies
correctness.
> * The timingsafe_bcmp function could be implemented using
> timingsafe_memcmp for the same reasons.
> * The compatibility wrapper headers doesn't check whether the standard
> library already provides prototypes. The #include_next mechanism is a
> good solution, but duplicate function prototypes would happen if the
> standard library happens to provide the extensions. Though, I think
> that should be harmless, I don't know how that interacts with
> __attribute__'s used in the system headers, if they are implemented as
> macros, or any other imaginable situation.
> * The <sys/stat.h> and <sys/time.h> inclusions in include/stdlib.h seems
> redundant.
> * The <sys/types.h> inclusion in string.h seems redundant, size_t is
> always provided by <string.h>.
Yes, likely some leaky BSDisms that we’ll fix upstream wherever is needed.
> * The header inclusion guard macro in include/machine/endian.h is
> _COMPAT_BYTE_ORDER_H_ (in the reserved namespace) while the other
> headers use the macro LIBCRYPTOCOMPAT_FOO_H.
> * The <stdint.h> inclusion in include/sys/types.h seems like a
> non-standard extension. Files relying on <sys/types.h> pulling in
> <stdint.h> should be rewritten to include <stdint.h> explicitly.
You’re right - that was to work around some earlier u_int definitions that
should probably be removed.
>
> Below you'll find the reasonably-finished parts of my local patch that
> solves some of the above issues.
>
> Thanks,
>
> Jonas 'Sortie' Termansen
>
> diff -Nur libressl-2.0.2/apps/Makefile.am libssl/apps/Makefile.am
> --- libressl-2.0.2/apps/Makefile.am 2014-07-16 05:25:52.000000000 +0200
> +++ libssl/apps/Makefile.am 2014-07-16 13:26:55.425448073 +0200
> @@ -4,8 +4,8 @@
>
> openssl_CFLAGS = $(USER_CFLAGS)
> openssl_LDADD = $(PLATFORM_LDADD)
> -openssl_LDADD += $(top_builddir)/crypto/libcrypto.la
> openssl_LDADD += $(top_builddir)/ssl/libssl.la
> +openssl_LDADD += $(top_builddir)/crypto/libcrypto.la
>
> openssl_SOURCES =
> noinst_HEADERS =
> diff -Nur libressl-2.0.2/apps/ocsp.c libssl/apps/ocsp.c
> --- libressl-2.0.2/apps/ocsp.c 2014-07-16 05:25:50.000000000 +0200
> +++ libssl/apps/ocsp.c 2014-07-16 13:26:55.429448073 +0200
> @@ -57,6 +57,8 @@
> */
> #ifndef OPENSSL_NO_OCSP
>
> +#include <sys/select.h>
> +
> #include <stdio.h>
> #include <stdlib.h>
> #include <limits.h>
> diff -Nur libressl-2.0.2/apps/openssl.c libssl/apps/openssl.c
> --- libressl-2.0.2/apps/openssl.c 2014-07-16 05:25:50.000000000 +0200
> +++ libssl/apps/openssl.c 2014-07-16 13:26:55.429448073 +0200
> @@ -109,7 +109,6 @@
> *
> */
>
> -#include <err.h>
> #include <signal.h>
> #include <stdio.h>
> #include <string.h>
> @@ -256,8 +255,10 @@
> arg.count = 0;
>
> bio_err = BIO_new_fp(stderr, BIO_NOCLOSE);
> - if (bio_err == NULL)
> - errx(1, "failed to initialise bio_err");
> + if (bio_err == NULL) {
> + fprintf(stderr, "%s: failed to initialize bio_err\n", argv[0]);
> + exit(1);
> + }
>
> CRYPTO_set_locking_callback(lock_dbg_cb);
>
> diff -Nur libressl-2.0.2/apps/s_client.c libssl/apps/s_client.c
> --- libressl-2.0.2/apps/s_client.c 2014-07-16 05:25:50.000000000 +0200
> +++ libssl/apps/s_client.c 2014-07-16 13:26:55.429448073 +0200
> @@ -137,6 +137,7 @@
>
> #include <sys/types.h>
> #include <sys/ioctl.h>
> +#include <sys/select.h>
> #include <sys/socket.h>
>
> #include <netinet/in.h>
> @@ -830,9 +831,8 @@
> BIO_printf(bio_c_out, "CONNECTED(%08X)\n", s);
>
> if (c_nbio) {
> - unsigned long l = 1;
> BIO_printf(bio_c_out, "turning on non blocking io\n");
> - if (BIO_socket_ioctl(s, FIONBIO, &l) < 0) {
> + if (BIO_socket_nbio(s, 1) < 0) {
> ERR_print_errors(bio_err);
> goto end;
> }
> diff -Nur libressl-2.0.2/apps/s_server.c libssl/apps/s_server.c
> --- libressl-2.0.2/apps/s_server.c 2014-07-16 05:25:50.000000000 +0200
> +++ libssl/apps/s_server.c 2014-07-16 13:26:55.429448073 +0200
> @@ -148,6 +148,7 @@
>
> #include <sys/types.h>
> #include <sys/ioctl.h>
> +#include <sys/select.h>
> #include <sys/socket.h>
>
> #include <assert.h>
> @@ -1363,11 +1364,9 @@
> goto err;
> }
> if (s_nbio) {
> - unsigned long sl = 1;
> -
> if (!s_quiet)
> BIO_printf(bio_err, "turning on non blocking io\n");
> - if (BIO_socket_ioctl(s, FIONBIO, &sl) < 0)
> + if (!BIO_socket_nbio(s, 1))
> ERR_print_errors(bio_err);
> }
>
> @@ -1798,11 +1797,9 @@
> goto err;
>
> if (s_nbio) {
> - unsigned long sl = 1;
> -
> if (!s_quiet)
> BIO_printf(bio_err, "turning on non blocking io\n");
> - if (BIO_socket_ioctl(s, FIONBIO, &sl) < 0)
> + if (!BIO_socket_nbio(s, 1))
> ERR_print_errors(bio_err);
> }
>
> diff -Nur libressl-2.0.2/apps/s_time.c libssl/apps/s_time.c
> --- libressl-2.0.2/apps/s_time.c 2014-07-16 05:25:50.000000000 +0200
> +++ libssl/apps/s_time.c 2014-07-16 13:26:55.433448073 +0200
> @@ -64,6 +64,7 @@
> -----------------------------------------*/
>
> #include <sys/socket.h>
> +#include <sys/select.h>
>
> #include <stdio.h>
> #include <stdlib.h>
> diff -Nur libressl-2.0.2/configure.ac libssl/configure.ac
> --- libressl-2.0.2/configure.ac 2014-07-16 05:25:48.000000000 +0200
> +++ libssl/configure.ac 2014-07-16 05:25:48.000000000 +0200
> @@ -25,7 +25,9 @@
> *openbsd*)
> AC_DEFINE([HAVE_ATTRIBUTE__BOUNDED__], [1], [OpenBSD gcc has
> bounded])
> ;;
> - *) ;;
> + *)
> + CFLAGS="$CFLAGS -D_ALL_SOURCE"
> + ;;
> esac
>
> AM_CONDITIONAL(TARGET_DARWIN, test x$TARGET_OS = xdarwin)
> diff -Nur libressl-2.0.2/crypto/bio/b_sock.c libssl/crypto/bio/b_sock.c
> --- libressl-2.0.2/crypto/bio/b_sock.c 2014-07-16 05:25:49.000000000
> +0200
> +++ libssl/crypto/bio/b_sock.c 2014-07-16 13:26:55.437448072 +0200
> @@ -65,6 +65,7 @@
> #include <netinet/tcp.h>
>
> #include <errno.h>
> +#include <fcntl.h>
> #include <limits.h>
> #include <netdb.h>
> #include <stdio.h>
> @@ -462,5 +463,10 @@
> int
> BIO_socket_nbio(int s, int mode)
> {
> - return (BIO_socket_ioctl(s, FIONBIO, &mode) == 0);
> + int flags = fcntl(s, F_GETFD);
> + if ( mode && !(flags & O_NONBLOCK) )
> + return (fcntl(s, F_SETFL, flags | O_NONBLOCK) == 0);
> + else if ( !mode && (flags & O_NONBLOCK) )
> + return (fcntl(s, F_SETFL, flags & ~O_NONBLOCK) == 0);
> + return 1;
> }
> diff -Nur libressl-2.0.2/crypto/err/err.c libssl/crypto/err/err.c
> --- libressl-2.0.2/crypto/err/err.c 2014-07-16 05:25:49.000000000 +0200
> +++ libssl/crypto/err/err.c 2014-07-16 13:26:55.437448072 +0200
> @@ -596,7 +596,7 @@
> if (str->string == NULL) {
> char (*dest)[LEN_SYS_STR_REASON] =
> &(strerror_tab[i - 1]);
> - char *src = strerror(i);
> + const char *src = strerror(i);
> if (src != NULL) {
> strlcpy(*dest, src, sizeof *dest);
> str->string = *dest;
> diff -Nur libressl-2.0.2/ssl/d1_lib.c libssl/ssl/d1_lib.c
> --- libressl-2.0.2/ssl/d1_lib.c 2014-07-16 05:25:48.000000000 +0200
> +++ libssl/ssl/d1_lib.c 2014-07-16 13:26:55.441448071 +0200
> @@ -57,7 +57,6 @@
> *
> */
>
> -#include <sys/param.h>
> #include <sys/socket.h>
>
> #include <netinet/in.h>
>
