Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate moving OS specific Socket and Thread to the Patch Page #208

Closed
noloader opened this issue Jun 29, 2016 · 5 comments
Closed

Investigate moving OS specific Socket and Thread to the Patch Page #208

noloader opened this issue Jun 29, 2016 · 5 comments

Comments

@noloader
Copy link
Collaborator

noloader commented Jun 29, 2016

Denis Bider offered some arguments to remove some code from the library at issue 23: Recent Windows 8 and Windows 10 support broke Windows 7/VS 2012. If I am parsing the comment correctly, DB suggests removing Socket, Pipe and Thread from the library. Its not clear if it includes the related Sources and Sinks.

I'd like to use this bug report to capture feedback, discover if anyone is using the classes, and track which files and classes are potential targets.

I know some of the classes are being used in the test program.


The Crypto++ wiki has a Patch Page. Its for components that may be useful to others, but don't quite fit into the library proper. If anything gets removed, then they surely would be placed there.

@zabulus
Copy link
Contributor

zabulus commented Jun 30, 2016

Fully agree. It makes much questions about what pipes, threads and so on are doing in crypto library.

@zabulus
Copy link
Contributor

zabulus commented Jun 30, 2016

Also cutting these parts will simplify further porting of stuff

@DevJPM
Copy link
Contributor

DevJPM commented Jun 30, 2016

TL;DR: I'm pro-removal.

A quick search showed that it was added in 4.0 and there's also a mention of sinks and sources in 5.0 related to sockets, so it's obviously only in the library so you can stream from a socket or a pipe into Crypto++'s filter system (decrypt, verify, decompress, decode it) and then have it at hand. So the original intent was to not limit people to files but also allow them to use advanced IPC and network communication methods with this fancy mechanism of filters, sources and sinks.

So far for the history lesson. Nowadays we don't need this any longer at network level. We have Boost.ASIO (or your favorite networking library) and we have libraries like OpenSSL, LibreSSL, BoringSSL, GnuTLS, s2n, cryptlib,... which can all handle TLS (a really good protocol for most use-cases) so I don't think we should encourage people here to "roll their own". And if people really need more lightweight encryption, there's our patch-page and of course cryptlib ;)

noloader added a commit to noloader/cryptopp that referenced this issue Aug 17, 2018
@noloader
Copy link
Collaborator Author

noloader commented Aug 17, 2018

Also see Remove Thread and Socket classes on the mailing list. We are testing the change now on a Testing fork.

The only sore spot was in fips140.cpp and the self-test code:

#if CRYPTOPP_ENABLE_COMPLIANCE_WITH_FIPS_140_2
ThreadLocalStorage & AccessPowerUpSelfTestInProgress()
{
	static ThreadLocalStorage selfTestInProgress;
	return selfTestInProgress;
}
#endif
bool PowerUpSelfTestInProgressOnThisThread()
{
#if CRYPTOPP_ENABLE_COMPLIANCE_WITH_FIPS_140_2
	return AccessPowerUpSelfTestInProgress().GetValue() != NULLPTR;
#else
	CRYPTOPP_ASSERT(false);	// should not be called
	return false;
#endif
}
void SetPowerUpSelfTestInProgressOnThisThread(bool inProgress)
{
	CRYPTOPP_UNUSED(inProgress);
#if CRYPTOPP_ENABLE_COMPLIANCE_WITH_FIPS_140_2
	AccessPowerUpSelfTestInProgress().SetValue((void *)inProgress);
#endif
}

It was changed to:

// One variable for all threads for compatibility. Previously this
// was a ThreadLocalStorage variable, which is per-thread. Also see
// https://github.com/weidai11/cryptopp/issues/208
static bool s_inProgress = false;

bool PowerUpSelfTestInProgressOnThisThread()
{
	return s_inProgress;
}

void SetPowerUpSelfTestInProgressOnThisThread(bool inProgress)
{
	s_inProgress = inProgress;
}

@noloader
Copy link
Collaborator Author

Sockets and Threads were removed at Commit f2171cbe2f49.

noloader added a commit that referenced this issue Dec 12, 2018
This should have been removed at GH #208, PR #703.
noloader added a commit that referenced this issue Feb 2, 2019
We should have removed the source files at GH #208, PR #703
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants