linuxdcpp-team team mailing list archive

[cologic <1909861@xxxxxxxxxxxxxxxxxx>]

It looks in DC++ on Windows, the only supported DC++ platform,
FileReader::readMapped() already always fell through to
FileReader::readCached():

                ret = readMapped(file, callback);

		if(ret == READ_FAILED) {
			dcdebug("Reading [full] %s\n", file.c_str());
			ret = readCached(file, callback);
		}

...

#ifdef _WIN32

...

size_t FileReader::readMapped(const string& file, const DataCallback& callback) {
	/** @todo mapped reads can fail on Windows by throwing an exception that may only be caught by
	SEH. MinGW doesn't have that, thus making this method of reading prone to unrecoverable
	failures. disabling this for now should be fine as DC++ always tries overlapped reads first
	(at the moment this file reader is only used in places where overlapped reads make the most
	sense).
	more info:
	<https://msdn.microsoft.com/en-us/library/aa366801(VS.85).aspx>
	<https://stackoverflow.com/q/7244645> */
#if 1
	return READ_FAILED;
#else
...
#endif
}

#else

...

static sigjmp_buf sb_env;

static void sigbus_handler(int signum, siginfo_t* info, void* context) {
	// Jump back to the readMapped which will return error. Apparently truncating
	// a file in Solaris sets si_code to BUS_OBJERR
	if (signum == SIGBUS && (info->si_code == BUS_ADRERR || info->si_code == BUS_OBJERR))
		siglongjmp(sb_env, 1);
}

size_t FileReader::readMapped(const string& filename, const DataCallback& callback) {
  ...
}

#endif

Had this also been the case in AirDC++ before you disabled
FileReader::readMapped() entirely, or were you only seeing reports of it
from non-Windows systems, such as the Gentoo bug you linked?

Either way, it seems like a safe, reasonable, risk-averse change, since
DC++ only targets Windows platforms currently, so at best there's a
bunch of dead code in one platform-dependent preprocessor branch and
some and very likely subtly incorrect code in the other platform-
dependent branch. I'm inclined to remove FileReader::readMapped(),
exactly as you suggest.

-- 
You received this bug notification because you are a member of
Dcplusplus-team, which is subscribed to DC++.
https://bugs.launchpad.net/bugs/1909861

Title:
  FileReader is not thread safe on Linux

Status in DC++:
  New

Bug description:
  FileReader::readMapped currently modifies the global SIGBUS handler in
  order to catch read errors:

  https://sourceforge.net/p/dcplusplus/code/ci/default/tree/dcpp/FileReader.cpp#l289

  Since the function can be called concurrently from different threads
  (currently hashing/queue recheck/sfv check in DC++) and each of them
  sets and resets the SIGBUS handler, there's a high risk that the
  application will crash in case of read errors as they aren't being
  handler properly.

  More information about the caveats:
  https://www.sublimetext.com/blog/articles/use-mmap-with-care

  These issues are much more likely to happen with AirDC++ as it uses
  multiple threads for hashing. Read errors caused rather nasty crashes
  with corrupted stack traces for one user, but luckily he was able to
  catch the SIGBUS signal with gdb.

  I didn't even spend time in trying to figure out how to make the
  mapped reads work properly, as based on my testing the basic
  FileReader::readCached function is noticeably faster:

  readMapped: 671 files (21.70 GiB) in 9 directories have been hashed in 4 minutes 21 seconds (84.87 MiB/s)
  readCached: 671 files (21.70 GiB) in 9 directories have been hashed in 3 minutes 58 seconds (93.08 MiB/s)

  FileReader::readMapped is now disabled in AirDC++, as I can't see any
  benefits from using it. The included setjmp.h header is even causing
  issues when using clang for compiling on Linux:
  https://bugs.gentoo.org/731676

To manage notifications about this bug go to:
https://bugs.launchpad.net/dcplusplus/+bug/1909861/+subscriptions