diff --git a/.github/workflows/unittest_linux.yml b/.github/workflows/unittest_linux.yml index b9afd118..c8328803 100644 --- a/.github/workflows/unittest_linux.yml +++ b/.github/workflows/unittest_linux.yml @@ -14,4 +14,4 @@ jobs: - uses: actions/checkout@v1 - uses: seanmiddleditch/gha-setup-ninja@master - name: make test - run: make -f makefile.dev test + run: make -f makefile.dev test WARNINGS_AS_ERRORS=ON diff --git a/CMakeLists.txt b/CMakeLists.txt index d4beeb05..f22b31ec 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -19,12 +19,20 @@ if (${CMAKE_SYSTEM_NAME} MATCHES "Linux") set(CMAKE_POSITION_INDEPENDENT_CODE ON) endif() -if (UNIX) - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic") -endif() - -if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wshorten-64-to-32") +option(IXWEBSOCKET_WARNINGS_AS_ERRORS "Treat IXWebSocket and ws warnings as errors" OFF) +set(IXWEBSOCKET_WARNING_FLAGS) +if (CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang" AND NOT MSVC) + if (UNIX) + list(APPEND IXWEBSOCKET_WARNING_FLAGS -Wall -Wextra -Wshadow -pedantic) + endif() + if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") + list(APPEND IXWEBSOCKET_WARNING_FLAGS -Wshorten-64-to-32) + endif() + if (IXWEBSOCKET_WARNINGS_AS_ERRORS) + list(APPEND IXWEBSOCKET_WARNING_FLAGS -Werror) + endif() +elseif (MSVC AND IXWEBSOCKET_WARNINGS_AS_ERRORS) + list(APPEND IXWEBSOCKET_WARNING_FLAGS /WX) endif() set( IXWEBSOCKET_SOURCES @@ -325,6 +333,8 @@ target_include_directories(ixwebsocket PUBLIC set_target_properties(ixwebsocket PROPERTIES PUBLIC_HEADER "${IXWEBSOCKET_HEADERS}") +target_compile_options(ixwebsocket PRIVATE ${IXWEBSOCKET_WARNING_FLAGS}) + add_library(ixwebsocket::ixwebsocket ALIAS ixwebsocket) option(IXWEBSOCKET_INSTALL "Install IXWebSocket" TRUE) diff --git a/docs/build.md b/docs/build.md index 90988041..307c34ef 100644 --- a/docs/build.md +++ b/docs/build.md @@ -69,6 +69,7 @@ Options for building: * `-DUSE_MBED_TLS=1` will use [mbedlts](https://tls.mbed.org/) for the TLS support * `-DUSE_WS=1` will build the ws interactive command line tool * `-DUSE_TEST=1` will build the unittest +* `-DIXWEBSOCKET_WARNINGS_AS_ERRORS=ON` treats library and CLI compiler warnings as errors. It is off by default; `make -f makefile.dev test WARNINGS_AS_ERRORS=ON` enables it and builds both targets, as used by Linux CI. Unix GCC/Clang builds enable `-Wshadow`. Warning flags are private to these targets and do not apply to vendored dependencies or consumers. The default CTest suite uses loopback servers and in-memory TLS sessions; it does not require Internet access. TLS server tests generate a local CA and verify the diff --git a/ixwebsocket/IXHttpClient.cpp b/ixwebsocket/IXHttpClient.cpp index 29302767..748797c0 100644 --- a/ixwebsocket/IXHttpClient.cpp +++ b/ixwebsocket/IXHttpClient.cpp @@ -264,14 +264,14 @@ namespace ix if (!success) { auto errorCode = args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::CannotConnect; - std::stringstream ss; - ss << "Cannot connect to url: " << url << " / error : " << errMsg; + std::ostringstream oss; + oss << "Cannot connect to url: " << url << " / error : " << errMsg; return std::make_shared(code, description, errorCode, headers, payload, - ss.str(), + oss.str(), uploadSize, downloadSize); } @@ -281,29 +281,23 @@ namespace ix if (args->verbose) { - std::stringstream ss; - ss << "Sending " << verb << " request " - << "to " << host << ":" << port << std::endl - << "request size: " << req.size() << " bytes" << std::endl - << "=============" << std::endl - << req << "=============" << std::endl - << std::endl; + std::ostringstream oss; + oss << "Sending " << verb << " request " + << "to " << host << ":" << port << std::endl + << "request size: " << req.size() << " bytes" << std::endl + << "=============" << std::endl + << req << "=============" << std::endl + << std::endl; - log(ss.str(), args); + log(oss.str(), args); } if (!_socket->writeBytes(req, isCancellationRequested)) { auto errorCode = args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::SendError; - std::string errorMsg("Cannot send request"); - return std::make_shared(code, - description, - errorCode, - headers, - payload, - errorMsg, - uploadSize, - downloadSize); + std::string msg("Cannot send request"); + return std::make_shared( + code, description, errorCode, headers, payload, msg, uploadSize, downloadSize); } uploadSize = req.size(); @@ -315,33 +309,27 @@ namespace ix if (!lineValid) { auto errorCode = args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::CannotReadStatusLine; - std::string errorMsg("Cannot retrieve status line"); - return std::make_shared(code, - description, - errorCode, - headers, - payload, - errorMsg, - uploadSize, - downloadSize); + std::string msg("Cannot retrieve status line"); + return std::make_shared( + code, description, errorCode, headers, payload, msg, uploadSize, downloadSize); } if (args->verbose) { - std::stringstream ss; - ss << "Status line " << line; - log(ss.str(), args); + std::ostringstream oss; + oss << "Status line " << line; + log(oss.str(), args); } if (sscanf(line.c_str(), "HTTP/1.1 %d", &code) != 1) { - std::string errorMsg("Cannot parse response code from status line"); + std::string msg("Cannot parse response code from status line"); return std::make_shared(code, description, HttpErrorCode::MissingStatus, headers, payload, - errorMsg, + msg, uploadSize, downloadSize); } @@ -353,15 +341,9 @@ namespace ix if (!headersValid) { auto errorCode = args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::HeaderParsingError; - std::string errorMsg("Cannot parse http headers"); - return std::make_shared(code, - description, - errorCode, - headers, - payload, - errorMsg, - uploadSize, - downloadSize); + std::string msg("Cannot parse http headers"); + return std::make_shared( + code, description, errorCode, headers, payload, msg, uploadSize, downloadSize); } // Redirect ? @@ -369,27 +351,27 @@ namespace ix { if (headers.find("Location") == headers.end()) { - std::string errorMsg("Missing location header for redirect"); + std::string msg("Missing location header for redirect"); return std::make_shared(code, description, HttpErrorCode::MissingLocation, headers, payload, - errorMsg, + msg, uploadSize, downloadSize); } if (redirects >= args->maxRedirects) { - std::stringstream ss; - ss << "Too many redirects: " << redirects; + std::ostringstream oss; + oss << "Too many redirects: " << redirects; return std::make_shared(code, description, HttpErrorCode::TooManyRedirects, headers, payload, - ss.str(), + oss.str(), uploadSize, downloadSize); } @@ -446,7 +428,7 @@ namespace ix else if (headers.find("Transfer-Encoding") != headers.end() && headers["Transfer-Encoding"] == "chunked") { - std::stringstream ss; + std::stringstream chunkSizeSs; while (true) { @@ -467,13 +449,13 @@ namespace ix } uint64_t chunkSize; - ss.str(""); - ss << std::hex << line; - ss >> chunkSize; + chunkSizeSs.str(""); + chunkSizeSs << std::hex << line; + chunkSizeSs >> chunkSize; if (args->verbose) { - std::stringstream oss; + std::ostringstream oss; oss << "Reading " << chunkSize << " bytes" << std::endl; log(oss.str(), args); } @@ -485,11 +467,12 @@ namespace ix isCancellationRequested); if (!chunkResult.first) { - auto errorCode = args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::ChunkReadError; + auto errCode = + args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::ChunkReadError; errorMsg = "Cannot read chunk"; return std::make_shared(code, description, - errorCode, + errCode, headers, payload, errorMsg, @@ -508,10 +491,11 @@ namespace ix if (!lineResult.first) { - auto errorCode = args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::ChunkReadError; + auto errCode = + args->cancel ? HttpErrorCode::Cancelled : HttpErrorCode::ChunkReadError; return std::make_shared(code, description, - errorCode, + errCode, headers, payload, errorMsg, @@ -528,13 +512,13 @@ namespace ix } else { - std::string errorMsg("Cannot read http body"); + std::string msg("Cannot read http body"); return std::make_shared(code, description, HttpErrorCode::CannotReadBody, headers, payload, - errorMsg, + msg, uploadSize, downloadSize); } @@ -548,25 +532,25 @@ namespace ix std::string decompressedPayload; if (!gzipDecompress(payload, decompressedPayload)) { - std::string errorMsg("Error decompressing payload"); + std::string msg("Error decompressing payload"); return std::make_shared(code, description, HttpErrorCode::Gzip, headers, payload, - errorMsg, + msg, uploadSize, downloadSize); } payload = decompressedPayload; #else - std::string errorMsg("ixwebsocket was not compiled with gzip support on"); + std::string msg("ixwebsocket was not compiled with gzip support on"); return std::make_shared(code, description, HttpErrorCode::Gzip, headers, payload, - errorMsg, + msg, uploadSize, downloadSize); #endif diff --git a/ixwebsocket/IXHttpServer.cpp b/ixwebsocket/IXHttpServer.cpp index 507edc52..11ca8b3c 100644 --- a/ixwebsocket/IXHttpServer.cpp +++ b/ixwebsocket/IXHttpServer.cpp @@ -133,7 +133,6 @@ namespace ix if (std::get<0>(ret)) { auto request = std::get<2>(ret); - std::shared_ptr response; // The Upgrade header value is case-insensitive (RFC 6455 4.2.1); // e.g. Chrome's remote-debugging proxy sends "Upgrade: WebSocket". const std::string& upgradeHeader = request->headers["Upgrade"]; diff --git a/ixwebsocket/IXSocketNetwork.cpp b/ixwebsocket/IXSocketNetwork.cpp index 0dcba213..f2d59917 100644 --- a/ixwebsocket/IXSocketNetwork.cpp +++ b/ixwebsocket/IXSocketNetwork.cpp @@ -328,7 +328,7 @@ namespace ix state->connection, 1, 65536, - ^(dispatch_data_t data, nw_content_context_t, bool complete, nw_error_t error) { + ^(dispatch_data_t data, nw_content_context_t, bool complete, nw_error_t readError) { auto s = weak.lock(); if (!s) return; std::lock_guard lock(s->mutex); @@ -349,7 +349,7 @@ namespace ix s->error = "Cannot map received TLS data"; } s->eof = complete; - if (error) s->error = networkError(error); + if (readError) s->error = networkError(readError); if (!s->input.empty() || s->eof || !s->error.empty()) s->signal.notify(); else diff --git a/ixwebsocket/IXWebSocketHandshake.cpp b/ixwebsocket/IXWebSocketHandshake.cpp index 3e242ba2..856c6f0f 100644 --- a/ixwebsocket/IXWebSocketHandshake.cpp +++ b/ixwebsocket/IXWebSocketHandshake.cpp @@ -188,11 +188,11 @@ namespace ix // HTTP/1.0 is too old. if (httpVersion != "HTTP/1.1") { - std::stringstream ss; - ss << "Expecting HTTP/1.1, got " << httpVersion << ". " - << "Rejecting connection to " << url << ", status: " << status - << ", HTTP Status line: " << line; - return WebSocketInitResult(false, status, ss.str()); + std::ostringstream oss; + oss << "Expecting HTTP/1.1, got " << httpVersion << ". " + << "Rejecting connection to " << url << ", status: " << status + << ", HTTP Status line: " << line; + return WebSocketInitResult(false, status, oss.str()); } auto result = parseHttpHeaders(_socket, isCancellationRequested); @@ -208,11 +208,11 @@ namespace ix // a redirection (like 301) if (status != 101) { - std::stringstream ss; - ss << "Expecting status 101 (Switching Protocol), got " << status - << " status connecting to " << url << ", HTTP Status line: " << line; + std::ostringstream oss; + oss << "Expecting status 101 (Switching Protocol), got " << status + << " status connecting to " << url << ", HTTP Status line: " << line; - return WebSocketInitResult(false, status, ss.str(), headers, path); + return WebSocketInitResult(false, status, oss.str(), headers, path); } // Check the presence of the connection field @@ -230,9 +230,9 @@ namespace ix // if (!insensitiveStringCompare(headers["connection"], "Upgrade")) { - std::stringstream ss; - ss << "Invalid connection value: " << headers["connection"]; - return WebSocketInitResult(false, status, ss.str()); + std::ostringstream oss; + oss << "Invalid connection value: " << headers["connection"]; + return WebSocketInitResult(false, status, oss.str()); } char output[29] = {}; diff --git a/ixwebsocket/IXWebSocketTransport.cpp b/ixwebsocket/IXWebSocketTransport.cpp index ac999cc6..35333f05 100644 --- a/ixwebsocket/IXWebSocketTransport.cpp +++ b/ixwebsocket/IXWebSocketTransport.cpp @@ -1160,7 +1160,7 @@ namespace ix } std::ptrdiff_t ret = 0; { - std::lock_guard lock(_socketMutex); + std::lock_guard socketLock(_socketMutex); ret = _socket->send((char*) _txbufPending.data() + _txbufPendingOffset, _txbufPending.size() - _txbufPendingOffset); } diff --git a/makefile.dev b/makefile.dev index 80ccad29..36eb33a1 100644 --- a/makefile.dev +++ b/makefile.dev @@ -13,6 +13,8 @@ all: brew install: brew +WARNINGS_AS_ERRORS ?= OFF + MBEDTLS_CMAKE_PREFIX := ifeq ($(shell uname),Darwin) MBEDTLS_CMAKE_PREFIX := -DCMAKE_PREFIX_PATH=$$(brew --prefix mbedtls@3) @@ -130,7 +132,7 @@ test_server: (cd test && npm i ws && node broadcast-server.js) test: - mkdir -p build && (cd build ; cmake -GNinja -DCMAKE_INSTALL_MESSAGE=LAZY -DCMAKE_UNITY_BUILD=ON -DCMAKE_BUILD_TYPE=Debug -DUSE_TLS=1 -DUSE_TEST=1 ..) + mkdir -p build && (cd build ; cmake -GNinja -DCMAKE_INSTALL_MESSAGE=LAZY -DCMAKE_UNITY_BUILD=ON -DCMAKE_BUILD_TYPE=Debug -DUSE_TLS=1 -DUSE_TEST=1 -DUSE_WS=1 -DIXWEBSOCKET_WARNINGS_AS_ERRORS=$(WARNINGS_AS_ERRORS) ..) (cd build ; ninja) (cd build ; ninja -v test) diff --git a/ws/CMakeLists.txt b/ws/CMakeLists.txt index 3324f2d4..a6c0f45b 100644 --- a/ws/CMakeLists.txt +++ b/ws/CMakeLists.txt @@ -6,11 +6,6 @@ cmake_minimum_required (VERSION 3.14) project (ws) -# There's -Weverything too for clang -if (NOT WIN32) - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic") -endif() - set (CMAKE_CXX_STANDARD 11) option(USE_TLS "Add TLS support" ON) @@ -21,6 +16,8 @@ include_directories(ws ..) add_executable(ws ws.cpp) +target_compile_options(ws PRIVATE ${IXWEBSOCKET_WARNING_FLAGS}) + # CLI11 is vendored as a single header in third_party, and included as a system # header so that its warnings do not show up in our builds target_include_directories(ws SYSTEM PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/../third_party) diff --git a/ws/ws.cpp b/ws/ws.cpp index 95af7fe7..a26a7386 100644 --- a/ws/ws.cpp +++ b/ws/ws.cpp @@ -237,7 +237,6 @@ namespace ix { _webSocket.setUrl(_url); - std::stringstream ss; log(std::string("Connecting to url: ") + _url); _webSocket.setOnMessageCallback([this](const ix::WebSocketMessagePtr& msg) { @@ -422,12 +421,12 @@ namespace ix int caseNumber = i; - std::stringstream ss; - ss << url << "/runCase?case=" << caseNumber << "&agent=ixwebsocket"; + std::ostringstream oss; + oss << url << "/runCase?case=" << caseNumber << "&agent=ixwebsocket"; - std::string url(ss.str()); + std::string caseUrl(oss.str()); - AutobahnTestCase testCase(url, quiet); + AutobahnTestCase testCase(caseUrl, quiet); if (!testCase.run()) { completed = false; @@ -506,7 +505,6 @@ namespace ix { _webSocket.setUrl(_url); - std::stringstream ss; log(std::string("Connecting to url: ") + _url); _webSocket.setOnMessageCallback([this](const WebSocketMessagePtr& msg) { @@ -759,7 +757,6 @@ namespace ix _webSocket.setPerMessageDeflateOptions(webSocketPerMessageDeflateOptions); } - std::stringstream ss; log(std::string("Connecting to url: ") + _url); _webSocket.setOnMessageCallback([this](const ix::WebSocketMessagePtr& msg) { @@ -1545,7 +1542,6 @@ namespace ix { _webSocket.setUrl(_url); - std::stringstream ss; log(std::string("Connecting to url: ") + _url); _webSocket.setOnMessageCallback( @@ -1930,7 +1926,6 @@ namespace ix _enablePerMessageDeflate, false, false, 15, 15); _webSocket.setPerMessageDeflateOptions(webSocketPerMessageDeflateOptions); - std::stringstream ss; log(std::string("ws_receive: Connecting to url: ") + _url); _webSocket.setOnMessageCallback([this](const ix::WebSocketMessagePtr& msg) { @@ -2103,7 +2098,6 @@ namespace ix _enablePerMessageDeflate, false, false, 15, 15); _webSocket.setPerMessageDeflateOptions(webSocketPerMessageDeflateOptions); - std::stringstream ss; log(std::string("ws_send: Connecting to url: ") + _url); _webSocket.setOnMessageCallback([this](const WebSocketMessagePtr& msg) { @@ -2503,24 +2497,29 @@ int main(int argc, char** argv) int runCount = 1; bool decompressGzipMessages = false; - auto addGenericOptions = [&pidfile](CLI::App* app) { - app->add_option("--pidfile", pidfile, "Pid file"); - }; + auto addGenericOptions = [&pidfile](CLI::App* subcommand) + { subcommand->add_option("--pidfile", pidfile, "Pid file"); }; - auto addTLSOptions = [&tlsOptions, &verifyNone](CLI::App* app) { - app->add_option( - "--cert-file", tlsOptions.certFile, "Path to the (PEM format) TLS cert file") + auto addTLSOptions = [&tlsOptions, &verifyNone](CLI::App* subcommand) + { + subcommand + ->add_option( + "--cert-file", tlsOptions.certFile, "Path to the (PEM format) TLS cert file") ->check(CLI::ExistingPath); - app->add_option("--key-file", tlsOptions.keyFile, "Path to the (PEM format) TLS key file") + subcommand + ->add_option("--key-file", tlsOptions.keyFile, "Path to the (PEM format) TLS key file") ->check(CLI::ExistingPath); - app->add_option("--ca-file", tlsOptions.caFile, "Path to the (PEM format) ca roots file") + subcommand + ->add_option("--ca-file", tlsOptions.caFile, "Path to the (PEM format) ca roots file") ->check(CLI::ExistingPath); - app->add_option("--ciphers", - tlsOptions.ciphers, - "A (comma/space/colon) separated list of ciphers to use for TLS"); - app->add_flag("--tls", tlsOptions.tls, "Enable TLS (server only)"); - app->add_flag("--verify_none", verifyNone, "Disable peer cert verification"); - app->add_flag("--disable-hostname-validation", tlsOptions.disable_hostname_validation, "Disable validation of certificates' hostnames"); + subcommand->add_option("--ciphers", + tlsOptions.ciphers, + "A (comma/space/colon) separated list of ciphers to use for TLS"); + subcommand->add_flag("--tls", tlsOptions.tls, "Enable TLS (server only)"); + subcommand->add_flag("--verify_none", verifyNone, "Disable peer cert verification"); + subcommand->add_flag("--disable-hostname-validation", + tlsOptions.disable_hostname_validation, + "Disable validation of certificates' hostnames"); }; app.add_flag("--version", version, "Print ws version");