Check for strnlen and provide it if it is not found #5340

strnlen is POSIX.1-2008. Thus on some older/incompatible systems, this function is not defined. E.g. current mingw (see http://sourceforge.net/p/mingw/bugs/1912/) or on older Mac OS X. Check for such conditions and use memchr based implementation instead. This should fix #5338.

In the mingw bug report there is also a free-standing version given: http://sourceforge.net/p/mingw/bugs/1912/?page=1 I'd feel safer using that if we can't rely on memchr being correct either.

static __inline__ size_t strnlen( const char *start, size_t maxlen )
{
  /* Determine the length of a NUL terminated string, subject
   * to a maximum permitted length constraint.
   */
  const char *stop = start;

  /* Scan at most maxlen bytes, seeking a NUL terminator;
   * note that we MUST enforce the length check, BEFORE the
   * terminator check, otherwise we could scan maxlen + 1
   * bytes, which POSIX forbids.
   */
  while( ((stop - start) < maxlen) && *stop )
    ++stop;

  /* Result is the number of non-NUL bytes actually scanned.
   */
  return stop - start;
}

BTW @cfields is util.cpp the right place for compatibility functions, or do we have a better place for those?

I'd prefer getting rid of strnlen completely... Writing such backwards compatible stuff can only bring in new issues. And in this case, both environments have their solutions (fork of mingw, updated Mac OS X/Xcode)...

i agree with @paveljanik. I don't think support for old or non-posix system should end up in core code.

@paveljanik so you're proposing that we just close this? Fine with me, too.

@laanwj I'd like to see the original problem fixed somehow ;) In this PR or elsewhere. The previous check before strnlen was OK. Is it acceptable for you to revert it and use "if" check as before?

No, it is not acceptable to revert it. The old code was incorrect.

As it seems, strnlen is only not supported on old and broken platforms, so IMO we should just keep it this way.

@laanwj we could revert it intelligently, of course ;)

No. Using POSIX functions should be ok.

Either we add a compatibility fallback for strnlen, or we don't, but that code is going to stay the same. I don't want to change everything down to the lowest common denominator of broken platforms.

@laanwj Agreed. So I think the compatibility fallback is the way to go (I really would like to see Bitcoin Core compilable there). @cfields I also do not like util.h/cpp for this. Maybe under compat/ somewhere/new files?

compat-way approach in https://github.com/paveljanik/bitcoin/compare/strnlen2

Agreed that somewhere in compat/ is better. We probably have a few other things like this (atoi64 as a quick bad example) that we could justify moving to a new dependency-less compat file.

@paveljanik I like that approach. A separate file just for that is a bit overkill, but it also ensures that unrelated junk won't end up there in the future. I don't mind either way.

@theuni Yes, having one file for one function is not optimal. Can you come up with a better name for it? ;-) misc_functions.cpp and waiting for atoi64 and others?

@paveljanik ACK. I like that approach. A function per file may be overkill, but it's better than stuffing everything in util.cpp. We can always group them later if there are a lot and it makes sense. Can you push that to this branch?

@ laanwj Done

@LinuXperia can you please verify with the standard mingw please?

ut ACK

Being nitpicky: It looks like compat.h needs to include config/bitcoin-config.h. We've probably gotten away with it only by accident (include ordering) in the past.

Also being nitpicky: if the intention here is to avoid lumping things together needlessly.. this now pulls compat.h in as a dependency for anything that requires strnlen. compat/strnlen.h would avoid that, if necessary.

@theuni yes, compat.h needs bitcoin-config.h. Even now, because there is:

// As Solaris does not have the MSG_NOSIGNAL flag for send(2) syscall, it is defined as 0
#if !defined(HAVE_MSG_NOSIGNAL) && !defined(MSG_NOSIGNAL)
#define MSG_NOSIGNAL 0
#endif

already. strnlen.h means, that everyone using strnlen needs to include it. I do not want that ;-)

@theuni bitcoin-config.h added. OK now?

Thanks. utACK.

[Strange - this only worked for me after manually rm src/bitcoin-config.h. Seemingly this file moved to src/config and my build was still picking up the old file]

For google-ability it was failing with the following error:

In file included from util.h:17:0,
                 from chainparamsbase.cpp:8:
compat.h:92:8: error: previous declaration of ‘size_t strnlen(const char*, size_t)’ with ‘C++’ linkage
 size_t strnlen( const char *start, size_t max_len);
        ^
In file included from /usr/include/c++/4.8/cstring:42:0,
                 from /usr/include/boost/filesystem/path.hpp:36,
                 from util.h:27,
                 from chainparamsbase.cpp:8:
/usr/include/string.h:407:74: error: conflicts with new declaration with ‘C’ linkage
      __THROW __attribute_pure__ __nonnull ((1));
                                                                          ^
/usr/include/string.h:407:74: error: declaration of ‘size_t strnlen(const char*, size_t) throw ()’ has a different exception specifier
In file included from util.h:17:0,
                 from chainparamsbase.cpp:8:
compat.h:92:8: error: from previous declaration ‘size_t strnlen(const char*, size_t)’
 size_t strnlen( const char *start, size_t max_len);
        ^

Problem is unrelated to this pull specifically though.

ACK commithash c8ed6130a4f7cfae897220ec093b440d21727cde https://dev.visucore.com/bitcoin/acks/5340

Isn't this because of this?

crypto/common.h:#include "bitcoin-config.h" netbase.cpp:#include "bitcoin-config.h"

@laanwj Very strange. bitcoin-compat.h was moved to a new path to avoid problems like that where a stale file could be found. I'm not following your compiler's logic, since util.h explicitly includes "config/bitcoin-config.h".

Unless it's a linker error due to netbase.cpp, that'd be different. (that one should be fixed either way).

No, it was not a linker error.

@paveljanik the first one is correct, the second should be fixed. crypto/ is setup to not have access to core headers by default, and uses -Iconfig/ so that it can read only config/bitcoin-config.h