detail::is_socket_alive() is not work for https connection

[chisheng]

is_socket_alive() is always return ture even though peer closed TLS and TCP connecton, I dont konwn how to detect whether TLS connection is alived.

[yhirose]

@chisheng thanks for the feedback. This method is not intended to be used by users, rather for internal use in cpp-httplib to detect a current socket is writable. That's why it's in the namespace detail.

Also there exist some unit cases where you can see it returns 'false'. One of them is ServerTest.ClientStop. Hope it helps.

[chisheng]

Sorry，I didn't explain my problem clearly.Pseudocode
int main()
{
httplib::SSLClient cli();
cli.post();//first request is success
sleep(60);//The server will close the connection after 60 seconds
cli.post()//second request will failed，is_socket_alive() return true for TLS socket ,it should reconnect TLS instead of use previous one.
}

[yhirose]

@chisheng thanks for the more details. I'll try to reproduce it.

sleep(60);//The server will close the connection after 60 seconds

Does it actually mean that your server will close within 60 seconds?

[chisheng]

yes,the keep-alive timeout of http server is 60s

[yhirose]

@chisheng I now confirmed it's a bug. Thanks for the report.

[yhirose]

@chisheng I fixed it. Could you try it with the latest httplib.h in your project?

[yhirose]

@chisheng it seems like my fix disable KeepAlive connection completely... Sorry about that. I'll work on it again.

[yhirose]

@chisheng I now fixed it. Could you try the latest httplib.h in the master branch? If it works on your machine, I'll bump up the version number. Thanks!

[oysteinmyrmo]

@yhirose I can confirm that c7e959 fixes the issues I mentioned in #1379. (Also, as you are aware of, ba5884 did not work very well.)

Thanks for the fix!

[chisheng]

@chisheng I now fixed it. Could you try the latest httplib.h in the master branch? If it works on your machine, I'll bump up the version number. Thanks!

sorry,I think it is still have a problem,I usring 0.9.7 because It wasn't implemented at the time that using post method with provicer ，I add extended post port with provider According to my situation in small Small memory device。

Function with provider may be not work ，beacause i find write_request return false，SSL_peek is not executed.and SSL_peek（）return 0 mean TLS broken? did not use SSL_get_error() to get error ,I guess SSL_get_error return SSL_ERROR_WANT_READ when The server response delay ，SSL_peek return 0 in this situation.

[yhirose]

@chisheng, thanks for the feedback.

I usring 0.9.7 because...

What does it mean by that? If it's not you, could tell the developer to try with the latest httplib.h in the master branch? 0.9.7 is too old, and I don't support it any more.

Function with provider may be not work ，beacause i find write_request return false

I can't reproduce the problem on my machine. Could you provide the smallest possible code example or a unit test, so that I can see what's going on? My test case in test/test.cc is KeepAliveTest.SSLClientReconnection.

SSL_peek（）return 0 mean TLS broken?

Not necessarily though, it's the only way to detect the SSL peer is closed, because when the client succeeds sending a HTTP request, it is supposed to receive a HTTP response according to the HTTP specification. But just in case, I added a code to check the error code from SSL_get_error, and let the client reconnect only when the error code is SSL_ERROR_ZERO_RETURN. 1ebb841

[chisheng]

Instead of GET request, Mey be use POST requests with both MultipartFormDataItems and ContentProviders can indicate the problem.I will do the test on x86 device using latest version，but it takes some time.Currently I am using this library in the embed arm linux system

[chisheng]

I think the most reasonable way to determine whether the ssl connection is broken by peer is to use the function SSL_ Peek() and SSL_ get_ Shutdown (), just as catboost used.

https://github.com/catboost/catboost/blob/master/library/cpp/neh/https.cpp

int PollReadT(const TDuration& timeout) {
if (!Connection_) {
return -1;
}

while (true) {
	const int rpoll = Connection_->PollT(CONT_POLL_READ, timeout);
	if (!Ssl_ || rpoll) {
		return rpoll;
	}

	char c = 0;
	const int rpeek = SSL_peek(Ssl_.Get(), &c, sizeof(c));
	if (rpeek < 0) {
		return -1;
	} else if (rpeek > 0) {
		return 0;
	} else {
		if ((SSL_get_shutdown(Ssl_.Get()) & SSL_RECEIVED_SHUTDOWN) != 0) {
			Shutdown(); // wait until shutdown is finished
			return EIO;
		}
	}
}

}

In addition, I don't think all compilers can guarantee SSL_ Peek() precedes SSL_ get_ Error () is called,in the following statement.
if (SSL_peek(socket_.ssl, buf, 1) == 0 && SSL_get_error(socket_.ssl, 0) == SSL_ERROR_ZERO_RETURN)

[yhirose]

In addition, I don't think all compilers can guarantee SSL_ Peek() precedes SSL_ get_ Error () is called,in the following statement.

Your saying isn't correct. Short-circuiting and evaluation order are required for operators || and && in both C and C++ standards.
https://stackoverflow.com/questions/628526/is-short-circuiting-logical-operators-mandated-and-evaluation-order

[chisheng]

Use the following unit test code to reproduce the bug

TEST(KeepAliveTest, SSLClientReconnection) {
SSLServer svr(SERVER_CERT_FILE, SERVER_PRIVATE_KEY_FILE);
ASSERT_TRUE(svr.is_valid());
svr.set_keep_alive_timeout(1);
std::string content="reconnect";

svr.Post("/hi", [](const httplib::Request &, httplib::Response &res) {
res.set_content("Hello World!", "text/plain");
});

auto f = std::async(std::launch::async, [&svr] { svr.listen(HOST, PORT); });
std::this_thread::sleep_for(std::chrono::milliseconds(200));

SSLClient cli(HOST, PORT);
cli.enable_server_certificate_verification(false);
cli.set_keep_alive(true);

auto result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
sink.write(content.c_str(),content.size());
return true;

},"text/plain");
ASSERT_TRUE(result);
EXPECT_EQ(200, result->status);

std::this_thread::sleep_for(std::chrono::seconds(2));

// Recoonect
result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
sink.write(content.c_str(),content.size());
return true;
},"text/plain");
ASSERT_TRUE(result);
EXPECT_EQ(200, result->status);

result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
sink.write(content.c_str(),content.size());

return true;
},"text/plain");
ASSERT_TRUE(result);
EXPECT_EQ(200, result->status);

svr.stop();
f.wait();
}

[yhirose]

I just tested with your unit test using Post method, but I don't see any problem with the latest httplib.h.

~/cpp-httplib/test$ make test && ./test --gtest_filter="*SSLClientReconnection*"
make: `test' is up to date.
Running main() from gtest/gtest_main.cc
Note: Google Test filter = *SSLClientReconnection*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from KeepAliveTest
[ RUN      ] KeepAliveTest.SSLClientReconnection
[       OK ] KeepAliveTest.SSLClientReconnection (3249 ms)
[----------] 1 test from KeepAliveTest (3249 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (3249 ms total)
[  PASSED  ] 1 test.

[chisheng]

Do you completely replace my unit test code or you just replace the original unit test get method with the post method? You must use the method with a provider to reproduce it

[yhirose]

Yes, I took your version of KeepAliveTest.SSLClientReconnection, and replaced my original version with yours completely.

[chisheng]

Strangely, I replaced a computer and still can reproduce this bug.In theory, if you use the request function with the provider parameter, write_ Request() returns false, resulting in SSL_ Peek() cannot be executed.

if (!write_request(strm, req, close_connection, error)) { return false; }//returned ,when call Post() with provider parameter,write_ Request() returns false

#ifdef CPPHTTPLIB_OPENSSL_SUPPORT
if (is_ssl()) {
char buf[1];
if (SSL_peek(socket_.ssl, buf, 1) == 0 &&
.............

chisheng@chisheng-EQ59:~/code/cpp-httplib/test$ make test && ./test --gtest_filter="SSLClientReconnection"
openssl genrsa 2048 > key.pem
openssl req -new -batch -config test.conf -key key.pem | openssl x509 -days 3650 -req -signkey key.pem > cert.pem
Certificate request self-signature ok
subject=C = US, ST = Test State or Province, L = Test Locality, O = Organization Name, OU = Organizational Unit Name, CN = Common Name, emailAddress = [email protected]
openssl req -x509 -config test.conf -key key.pem -sha256 -days 3650 -nodes -out cert2.pem -extensions SAN
openssl genrsa 2048 > rootCA.key.pem
openssl req -x509 -new -batch -config test.rootCA.conf -key rootCA.key.pem -days 1024 > rootCA.cert.pem
openssl genrsa 2048 > client.key.pem
openssl req -new -batch -config test.conf -key client.key.pem | openssl x509 -days 370 -req -CA rootCA.cert.pem -CAkey rootCA.key.pem -CAcreateserial > client.cert.pem
Certificate request self-signature ok
subject=C = US, ST = Test State or Province, L = Test Locality, O = Organization Name, OU = Organizational Unit Name, CN = Common Name, emailAddress = [email protected]
openssl genrsa -passout pass:test123! 2048 > key_encrypted.pem
openssl req -new -batch -config test.conf -key key_encrypted.pem | openssl x509 -days 3650 -req -signkey key_encrypted.pem > cert_encrypted.pem
Certificate request self-signature ok
subject=C = US, ST = Test State or Province, L = Test Locality, O = Organization Name, OU = Organizational Unit Name, CN = Common Name, emailAddress = [email protected]
#c_rehash .
clang++ -o test -I.. -g -std=c++11 -I. -Wall -Wextra -Wtype-limits -Wconversion -Wshadow test.cc include_httplib.cc gtest/gtest-all.cc gtest/gtest_main.cc -DCPPHTTPLIB_OPENSSL_SUPPORT -I/usr/local/opt/[email protected]/include -L/usr/local/opt/[email protected]/lib -lssl -lcrypto -DCPPHTTPLIB_ZLIB_SUPPORT -lz -DCPPHTTPLIB_BROTLI_SUPPORT -I/usr/local/opt/brotli/include -L/usr/local/opt/brotli/lib -lbrotlicommon -lbrotlienc -lbrotlidec -pthread
test.cc:4181:63: warning: unused parameter 'offset' [-Wunused-parameter]
auto result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
^
test.cc:4181:78: warning: unused parameter 'length' [-Wunused-parameter]
auto result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
^
test.cc:4192:58: warning: unused parameter 'offset' [-Wunused-parameter]
result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
^
test.cc:4192:73: warning: unused parameter 'length' [-Wunused-parameter]
result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
^
test.cc:4199:58: warning: unused parameter 'offset' [-Wunused-parameter]
result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
^
test.cc:4199:73: warning: unused parameter 'length' [-Wunused-parameter]
result = cli.Post("/hi",content.size(),[&content](size_t offset, size_t length, DataSink &sink){
^
In file included from test.cc:4:
./gtest/gtest.h:11427:11: warning: comparison of integers of different signs: 'const unsigned long' and 'const int' [-Wsign-compare]
if (lhs == rhs) {
~~~ ^ ~~~
./gtest/gtest.h:11446:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<unsigned long, int>' requested here
return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
^
test.cc:1836:19: note: in instantiation of function template specialization 'testing::internal::EqHelper::Compare<unsigned long, int, nullptr>' requested here
EXPECT_EQ(text_value.size(), 1);
^
./gtest/gtest.h:11926:54: note: expanded from macro 'EXPECT_EQ'
EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2)
^
7 warnings generated.
Running main() from gtest/gtest_main.cc
Note: Google Test filter = SSLClientReconnection
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from KeepAliveTest
[ RUN ] KeepAliveTest.SSLClientReconnection
test.cc:4196: Failure
Value of: result
Actual: false
Expected: true

[yhirose]

@chisheng according to #1527 including your test, it looks like my solution only works on Windows and macOS, but not on linux... When I have time, I'll look into it.

[yhirose]

This now should be fixed.