[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)

2020-08-06 Thread GitBox


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
 int CRCStream::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(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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)

2020-08-05 Thread GitBox


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(const_cast(packet->payload_.c_str())), len);
-if (ret != (int64_t)len) {
+ret = transaction->getStream().writeData(reinterpret_cast(const_cast(packet->payload_.c_str())),
+ gsl::narrow(len));
+if (ret != static_cast(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 ,
 const std::vector _body = client.getResponseBody();
 const std::vector _headers = client.getHeaders();
 
-int64_t http_code = client.getResponseCode();
+int http_code = gsl::narrow(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 expr_substring(const std::vector ) {
   if (args.size() < 3) {
-return Value(args[0].asString().substr(args[1].asUnsignedLong()));
+size_t offset = gsl::narrow(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(args[1].asUnsignedLong());
+size_t count = gsl::narrow(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
 int CRCStream::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(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:
us...@infra.apache.org




[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #848: MINIFICPP-1268 Fix compiler warnings on Windows (VS2017)

2020-08-05 Thread GitBox


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 expr_substring(const std::vector ) {
   if (args.size() < 3) {
-return Value(args[0].asString().substr(args[1].asUnsignedLong()));
+size_t offset = gsl::narrow(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(args[1].asUnsignedLong());
+size_t count = gsl::narrow(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:
us...@infra.apache.org