On 2022-01-04 at 21:01:19, Junio C Hamano wrote:

"brian m. carlson" [off-list ref] writes:

quoted

+ifeq ($(strip $(CSPRNG_METHOD)),arc4random)
+	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
+endif
+
+ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd)
+	BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD
+	EXTLIBS += -lbsd
+endif
+
+ifeq ($(strip $(CSPRNG_METHOD)),getrandom)
+	BASIC_CFLAGS += -DHAVE_GETRANDOM
+endif
+
+ifeq ($(strip $(CSPRNG_METHOD)),getentropy)
+	BASIC_CFLAGS += -DHAVE_GETENTROPY
+endif
+
+ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom)
+	BASIC_CFLAGS += -DHAVE_RTLGENRANDOM
+endif
+
+ifeq ($(strip $(CSPRNG_METHOD)),openssl)
+	BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG
+endif

Use of $(strip ($VAR)) looks a bit different from what everybody
else does with ifeq in this Makefile.  Was there a particular reason
to use it that I am missing?

As far as I'm aware, ifeq includes whitespace in its comparisons (at
least, I was led to believe that by the documentation for GNU make).
However, in many places, we insert leading spaces before the variable
name.  Some testing shows that GNU make strips those off, although my
earlier testing led me to believe otherwise.

So I'll change this in v3.

When we see something unrecognized in CSPRNG_METHOD, we do not touch
BASIC_CFLAGS (or EXTLIBS) here.  I wonder how easy a clue we would
have to decide that we need to fall back to urandom.  IOW, I would
have expected a if/else if/... cascade that has "no we do not have
anything else and need to fall back to urandom" at the end.

I tried to avoid the if/else if cascade for the same reasons that we do
in config.mak.uname, which is that it gets pretty hard to reason about
after you have more than just a few items.

quoted

diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c
new file mode 100644
index 0000000000..65d14973c5
--- /dev/null
+++ b/t/helper/test-csprng.c

@@ -0,0 +1,29 @@
+#include "test-tool.h"
+#include "git-compat-util.h"
+
+
+int cmd__csprng(int argc, const char **argv)
+{
+	unsigned long count;
+	unsigned char buf[1024];
+
+	if (argc > 2) {
+		fprintf(stderr, "usage: %s [<size>]\n", argv[0]);
+		return 2;
+	}
+
+	count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L;
+
+	while (count) {
+		unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);

"chunk" should be of type "size_t", no?

Yes, it should.  Will fix in v3.

quoted

diff --git a/wrapper.c b/wrapper.c
index 36e12119d7..1052356703 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags)
 	return open(path, flags);
 #endif
 }
+
+int csprng_bytes(void *buf, size_t len)
+{
+#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD)

Shouldn't HAVE_ARC4RANDOM mean "we have arc4random_buf() function
available; please use that.", i.e. wouldn't this give us cleaner
result in the change to the Makefile?

 ifeq ($(strip $(CSPRNG_METHOD)),arc4random)
 	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
 endif
 
 ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd)
-	BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD
+	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
 	EXTLIBS += -lbsd
 endif

To put it differently, C source, via BASIC_CFLAGS, would not have to
care where the function definition comes from.  It is linker's job
to care and we are already telling it via EXTLIBS, so I am not sure
the value of having HAVE_ARC4RANDOM_LIBBSD as a separate symbol.

There's an important additional difference here.  On real BSD systems,
the prototypes for this family of functions live in <stdlib.h>.
Obviously, on Linux with libbsd, that's not the case.  So, in
git-compat-util.h, we need to include <bsd/stdlib.h> to get the
prototype, and that's done only if HAVE_ARC4RANDOM_LIBBSD is set, since
on a real BSD system, that header isn't present.

If you'd prefer, I can set both flags here, one which controls the
function use and one which controls the #define, or I can leave it as it
is.

OK, I earlier worried about the lack of explicit "we are using
urandom" at the Makefile level, but as long as this will remain the
only place that needs to care about the fallback, the above is
perfectly fine.

Yes, I tried very hard to make this the only place the default mattered.

And, for the record, I think /dev/urandom is the sensible default here,
because I know it exists on some of the proprietary Unix systems, like
Solaris, where I believe it originated, and QNX, as well as Linux and
the BSDs.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA