You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/07/23 12:53:23 UTC
[GitHub] [nifi-minifi-cpp] fgerlits opened a new pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
fgerlits opened a new pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848
Change the warning level from W1 (severe warnings only) to W3 (production quality warnings).
The largest batch of changes is due to `uint64_t` -> `size_t` conversions, which are narrowing on Windows because we build a 32-bit binary.
Almost all changes should have no effect on behavior, except when a narrowing would occur. The two exceptions are:
* in `C2Agent::configure()` (C2Agent.cpp:256) the value of the property used to be read and discarded; now it is assigned to a field (the correct one, I hope);
* in `WriteCallback::process()` (SiteToSiteClient.h:308) we calculated the number of bytes received, but then we did not log it but always logged 0 instead.
I did not deal with warnings related to the min/max macros, as those will be fixed by #837
---
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r463060513
##########
File path: libminifi/include/io/Serializable.h
##########
@@ -44,8 +44,9 @@ Integral byteSwap(Integral i) {
#ifdef htonll
return htonll(i);
#else
- #define htonll_r(x) ((((uint64_t)htonl(x)) << 32) + htonl((x) >> 32))
- return htonll_r(i);
+ uint32_t lower_32_bits = static_cast<uint32_t>(i);
Review comment:
similarly, the other `byteSwap` overloads all seem to be noop on big-endian systems
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r465978905
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -224,7 +222,7 @@ template<typename T>
int CRCStream<T>::readData(uint8_t *buf, int buflen) {
int ret = child_stream_->read(buf, buflen);
if (ret > 0) {
- crc_ = crc32(crc_, buf, ret);
+ crc_ = crc32(gsl::narrow<uLong>(crc_), buf, ret);
Review comment:
`uLong` in zlib is a typedef to `unsigned long`, so on 64-bit Linux it is 64 bits long. I have changed the type of `crc_` to `uLong`, so now we only have one `gsl::narrow` (called once per instance) instead of three (called many times).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r465978905
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -224,7 +222,7 @@ template<typename T>
int CRCStream<T>::readData(uint8_t *buf, int buflen) {
int ret = child_stream_->read(buf, buflen);
if (ret > 0) {
- crc_ = crc32(crc_, buf, ret);
+ crc_ = crc32(gsl::narrow<uLong>(crc_), buf, ret);
Review comment:
`uLong` in zlib is a typedef to `unsigned long`, so on 64-bit Linux it is 64 bits. I have changed the type of `crc_` to `uLong`, so now we only have one `gsl::narrow` (called once per instance) instead of three (called many times).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r465979548
##########
File path: extensions/http-curl/processors/InvokeHTTP.cpp
##########
@@ -337,7 +337,7 @@ void InvokeHTTP::onTrigger(const std::shared_ptr<core::ProcessContext> &context,
const std::vector<char> &response_body = client.getResponseBody();
const std::vector<std::string> &response_headers = client.getHeaders();
- int64_t http_code = client.getResponseCode();
+ int http_code = gsl::narrow<int>(client.getResponseCode());
Review comment:
I have removed this `gsl::narrow()` call.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r463113874
##########
File path: extensions/libarchive/BinFiles.h
##########
@@ -91,7 +92,7 @@ class Bin {
if (flow->getAttribute(fileCount_, value)) {
try {
// for defrag case using the identification
- int count = std::stoi(value);
+ size_t count = std::stoul(value);
Review comment:
I wanted to keep changes minimal, just making sure warnings, especially warnings about implicit narrowing, are gone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r466282492
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -224,7 +222,7 @@ template<typename T>
int CRCStream<T>::readData(uint8_t *buf, int buflen) {
int ret = child_stream_->read(buf, buflen);
if (ret > 0) {
- crc_ = crc32(crc_, buf, ret);
+ crc_ = crc32(gsl::narrow<uLong>(crc_), buf, ret);
Review comment:
Oh, I assumed it must be 32 bit, because it's crc32. Great, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r463111602
##########
File path: libminifi/include/io/Serializable.h
##########
@@ -44,8 +44,9 @@ Integral byteSwap(Integral i) {
#ifdef htonll
return htonll(i);
#else
- #define htonll_r(x) ((((uint64_t)htonl(x)) << 32) + htonl((x) >> 32))
- return htonll_r(i);
+ uint32_t lower_32_bits = static_cast<uint32_t>(i);
Review comment:
The new code is equivalent to the old code which used the `#define`, but you are right that it's not always equivalent to `htonll`. We check `is_little_endian` every time before we call `byteSwap`; it might be better to move that check inside `byteSwap`, but I didn't think that was in scope for this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r463040371
##########
File path: extensions/libarchive/BinFiles.h
##########
@@ -91,7 +92,7 @@ class Bin {
if (flow->getAttribute(fileCount_, value)) {
try {
// for defrag case using the identification
- int count = std::stoi(value);
+ size_t count = std::stoul(value);
Review comment:
could use
```
uint32_t count;
utils::internal::ValueParser(value).parse(count).parseEnd()
```
##########
File path: extensions/libarchive/BinFiles.h
##########
@@ -91,7 +92,7 @@ class Bin {
if (flow->getAttribute(fileCount_, value)) {
try {
// for defrag case using the identification
- int count = std::stoi(value);
+ size_t count = std::stoul(value);
Review comment:
no `size_t` overload unfortunately
##########
File path: extensions/civetweb/tests/ListenHTTPTests.cpp
##########
@@ -169,7 +168,6 @@ class ListenHTTPTestsFixture {
}
protected:
- char* tmp_dir_format;
Review comment:
👍
##########
File path: libminifi/include/io/Serializable.h
##########
@@ -44,8 +44,9 @@ Integral byteSwap(Integral i) {
#ifdef htonll
return htonll(i);
#else
- #define htonll_r(x) ((((uint64_t)htonl(x)) << 32) + htonl((x) >> 32))
- return htonll_r(i);
+ uint32_t lower_32_bits = static_cast<uint32_t>(i);
Review comment:
are these two implementations equivalent? on big-endian systems `htonll` is noop while the other one swaps the bytes anyway
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r463404637
##########
File path: libminifi/include/io/Serializable.h
##########
@@ -44,8 +44,9 @@ Integral byteSwap(Integral i) {
#ifdef htonll
return htonll(i);
#else
- #define htonll_r(x) ((((uint64_t)htonl(x)) << 32) + htonl((x) >> 32))
- return htonll_r(i);
+ uint32_t lower_32_bits = static_cast<uint32_t>(i);
Review comment:
agree that it is not in scope
👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r465624329
##########
File path: libminifi/src/sitetosite/SiteToSiteClient.cpp
##########
@@ -499,8 +499,9 @@ int16_t SiteToSiteClient::send(std::string transactionID, DataPacket *packet, co
return -1;
}
- ret = transaction->getStream().writeData(reinterpret_cast<uint8_t *>(const_cast<char*>(packet->payload_.c_str())), len);
- if (ret != (int64_t)len) {
+ ret = transaction->getStream().writeData(reinterpret_cast<uint8_t *>(const_cast<char*>(packet->payload_.c_str())),
+ gsl::narrow<int>(len));
+ if (ret != static_cast<int64_t>(len)) {
Review comment:
uint64_t to int64_t is a narrowing conversion
##########
File path: extensions/http-curl/processors/InvokeHTTP.cpp
##########
@@ -337,7 +337,7 @@ void InvokeHTTP::onTrigger(const std::shared_ptr<core::ProcessContext> &context,
const std::vector<char> &response_body = client.getResponseBody();
const std::vector<std::string> &response_headers = client.getHeaders();
- int64_t http_code = client.getResponseCode();
+ int http_code = gsl::narrow<int>(client.getResponseCode());
Review comment:
I would check the response code before assuming that it's valid. I'm not sure whether curl validates the number before returning, but I wouldn't want a malicious server or MITM to be able to crash (`std::terminate` on narrowing failure) the minifi c++ agent.
##########
File path: nanofi/src/core/cstream.c
##########
@@ -153,9 +148,9 @@ int readUTFLen(uint32_t * utflen, cstream * stream) {
return ret;
}
-int readUTF(char * buf, uint64_t buflen, cstream * stream) {
+int readUTF(char * buf, uint32_t buflen, cstream * stream) {
Review comment:
Why uint32_t? Why not e.g. size_t or int?
##########
File path: extensions/expression-language/Expression.cpp
##########
@@ -194,9 +194,12 @@ Value expr_toLower(const std::vector<Value> &args) {
Value expr_substring(const std::vector<Value> &args) {
if (args.size() < 3) {
- return Value(args[0].asString().substr(args[1].asUnsignedLong()));
+ size_t offset = gsl::narrow<size_t>(args[1].asUnsignedLong());
+ return Value(args[0].asString().substr(offset));
} else {
- return Value(args[0].asString().substr(args[1].asUnsignedLong(), args[2].asUnsignedLong()));
+ size_t offset = gsl::narrow<size_t>(args[1].asUnsignedLong());
+ size_t count = gsl::narrow<size_t>(args[2].asUnsignedLong());
+ return Value(args[0].asString().substr(offset, count));
Review comment:
:+1:
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -224,7 +222,7 @@ template<typename T>
int CRCStream<T>::readData(uint8_t *buf, int buflen) {
int ret = child_stream_->read(buf, buflen);
if (ret > 0) {
- crc_ = crc32(crc_, buf, ret);
+ crc_ = crc32(gsl::narrow<uLong>(crc_), buf, ret);
Review comment:
I suggest changing the type of `crc_` to `uint32_t` and `static_assert` that `uLong` has the same width and signedness as `uint32_t`. This should make `gsl::narrow` calls redundant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r465977037
##########
File path: nanofi/src/core/cstream.c
##########
@@ -153,9 +148,9 @@ int readUTFLen(uint32_t * utflen, cstream * stream) {
return ret;
}
-int readUTF(char * buf, uint64_t buflen, cstream * stream) {
+int readUTF(char * buf, uint32_t buflen, cstream * stream) {
Review comment:
Because everywhere we call this function, we call it with an argument of type `uint32_t`, and the return value (output argument) of `readUTFLen()` is also of type `uint32_t`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r465979206
##########
File path: libminifi/src/sitetosite/SiteToSiteClient.cpp
##########
@@ -499,8 +499,9 @@ int16_t SiteToSiteClient::send(std::string transactionID, DataPacket *packet, co
return -1;
}
- ret = transaction->getStream().writeData(reinterpret_cast<uint8_t *>(const_cast<char*>(packet->payload_.c_str())), len);
- if (ret != (int64_t)len) {
+ ret = transaction->getStream().writeData(reinterpret_cast<uint8_t *>(const_cast<char*>(packet->payload_.c_str())),
+ gsl::narrow<int>(len));
+ if (ret != static_cast<int64_t>(len)) {
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r463111602
##########
File path: libminifi/include/io/Serializable.h
##########
@@ -44,8 +44,9 @@ Integral byteSwap(Integral i) {
#ifdef htonll
return htonll(i);
#else
- #define htonll_r(x) ((((uint64_t)htonl(x)) << 32) + htonl((x) >> 32))
- return htonll_r(i);
+ uint32_t lower_32_bits = static_cast<uint32_t>(i);
Review comment:
The new code is equivalent to the old code which used the `#define`, but you are right that it's not always equivalent to `htonll`. We check `is_little_endian` every time before we call `byteSwap`; it would probably be better to move that check inside `byteSwap`, but I didn't think that was in scope for this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #848:
URL: https://github.com/apache/nifi-minifi-cpp/pull/848#discussion_r463077312
##########
File path: extensions/expression-language/Expression.cpp
##########
@@ -194,9 +194,12 @@ Value expr_toLower(const std::vector<Value> &args) {
Value expr_substring(const std::vector<Value> &args) {
if (args.size() < 3) {
- return Value(args[0].asString().substr(args[1].asUnsignedLong()));
+ size_t offset = gsl::narrow<size_t>(args[1].asUnsignedLong());
+ return Value(args[0].asString().substr(offset));
} else {
- return Value(args[0].asString().substr(args[1].asUnsignedLong(), args[2].asUnsignedLong()));
+ size_t offset = gsl::narrow<size_t>(args[1].asUnsignedLong());
+ size_t count = gsl::narrow<size_t>(args[2].asUnsignedLong());
+ return Value(args[0].asString().substr(offset, count));
Review comment:
:+1: for naming the integers
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org