[
https://issues.apache.org/jira/browse/GEODE-10098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17501540#comment-17501540
]
ASF GitHub Bot commented on GEODE-10098:
----------------------------------------
pivotal-jbarrett commented on a change in pull request #943:
URL: https://github.com/apache/geode-native/pull/943#discussion_r819920150
##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -1373,14 +1360,10 @@ void TcrMessage::handleByteArrayResponse(
case TcrMessage::LOCAL_CREATE:
case TcrMessage::LOCAL_UPDATE: {
- int32_t regionLen = input.readInt32();
+ m_regionName.resize(input.readInt32());
Review comment:
What happens in the off chance this read a negative number?
##########
File path: cppcache/src/TcrConnection.cpp
##########
@@ -659,16 +656,15 @@ char* TcrConnection::readMessage(size_t* recvLen,
if (isNotificationMessage) {
// fix #752 - do not throw periodic TimeoutException for subscription
// channels to avoid frequent stack trace processing.
- return nullptr;
+ return {nullptr, 0, error};
Review comment:
I love tuple!
##########
File path: cppcache/src/util/make_unique.hpp
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef UTIL_MAKE_UNIQUE_H
+#define UTIL_MAKE_UNIQUE_H
+
+#include <cstddef>
+#include <memory>
+#include <type_traits>
+#include <utility>
+
+// N3656 standard revision of make_unique.
+
Review comment:
Should we put this in a namespace?
##########
File path: cppcache/src/CacheXmlParser.cpp
##########
@@ -494,8 +491,7 @@ void CacheXmlParser::setAttributes(Cache *) {}
*
*/
void CacheXmlParser::create(Cache *cache) {
- // use DeleteObject class to delete cacheCreation_ in case of exceptions
- DeleteObject<CacheXmlCreation> delCacheCreation(cacheCreation_);
+ std::unique_ptr<CacheXmlCreation> delCacheCreation(cacheCreation_);
Review comment:
Yes, It looks like the purpose here was to prevent the `cache` from
being leaked if there was an exception. I suspect the caller is already doing
this if it's a smart pointer already. Perhaps a `Cache&` is even more
appropriate to void even needing the `nullptr` check.
##########
File path: cppcache/src/TcrConnection.cpp
##########
@@ -531,11 +532,9 @@ ConnErrType TcrConnection::sendData(const char* buffer,
size_t length,
return CONN_NOERR;
}
-char* TcrConnection::sendRequest(const char* buffer, size_t len,
- size_t* recvLen,
- std::chrono::microseconds sendTimeoutSec,
- std::chrono::microseconds receiveTimeoutSec,
- int32_t request) {
+std::tuple<std::unique_ptr<char[]>, std::size_t> TcrConnection::sendRequest(
Review comment:
Why not `std::vector`, use that in several other places for this exact
same purpose.
##########
File path: cppcache/src/TcrConnection.cpp
##########
@@ -686,30 +682,27 @@ char* TcrConnection::readMessage(size_t* recvLen,
reinterpret_cast<uint8_t*>(msg_header), HEADER_LENGTH);
// ignore msgType
input.readInt32();
- msgLen = input.readInt32();
+ auto msgLen = input.readInt32();
// check that message length is valid.
- if (!(msgLen > 0) && request == TcrMessage::GET_CLIENT_PR_METADATA) {
- char* fullMessage;
- *recvLen = HEADER_LENGTH + msgLen;
- _GEODE_NEW(fullMessage, char[HEADER_LENGTH + msgLen]);
- std::memcpy(fullMessage, msg_header, HEADER_LENGTH);
- return fullMessage;
- // exit(0);
+ if (msgLen <= 0 && request == TcrMessage::GET_CLIENT_PR_METADATA) {
+ auto fullMessage = make_unique<char[]>(sizeof(msg_header));
+ std::memcpy(fullMessage.get(), msg_header, sizeof(msg_header));
+ return {std::move(fullMessage), sizeof(msg_header), CONN_NOERR};
}
- // user has to delete this pointer
- char* fullMessage;
- *recvLen = HEADER_LENGTH + msgLen;
- _GEODE_NEW(fullMessage, char[HEADER_LENGTH + msgLen]);
- std::memcpy(fullMessage, msg_header, HEADER_LENGTH);
+ auto recvLen = HEADER_LENGTH + msgLen;
+ auto fullMessage = make_unique<char[]>(recvLen);
+ std::memcpy(fullMessage.get(), msg_header, HEADER_LENGTH);
std::chrono::microseconds mesgBodyTimeout = receiveTimeoutSec;
if (isNotificationMessage) {
mesgBodyTimeout = receiveTimeoutSec * DEFAULT_TIMEOUT_RETRIES;
}
- error = receiveData(fullMessage + HEADER_LENGTH, msgLen, mesgBodyTimeout);
+ error =
+ receiveData(fullMessage.get() + HEADER_LENGTH, msgLen, mesgBodyTimeout);
if (error != CONN_NOERR) {
- delete[] fullMessage;
+ fullMessage.reset();
Review comment:
Agreed, let the smart pointer do the work later. It doesn't do anything
for us really to do it here explicitly.
##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -1373,14 +1360,10 @@ void TcrMessage::handleByteArrayResponse(
case TcrMessage::LOCAL_CREATE:
case TcrMessage::LOCAL_UPDATE: {
- int32_t regionLen = input.readInt32();
+ m_regionName.resize(input.readInt32());
input.advanceCursor(1); // ignore byte
- char* regname = nullptr;
- regname = new char[regionLen + 1];
- DeleteArray<char> delRegName(regname);
- input.readBytesOnly(reinterpret_cast<int8_t*>(regname), regionLen);
- regname[regionLen] = '\0';
- m_regionName = regname;
+ input.readBytesOnly(reinterpret_cast<int8_t*>(&m_regionName[0]),
Review comment:
Seems very unsafe! Do we know that all implementations of `std::string`
have allocated a contiguous space large enough to do this? There is a reason
that `c_str()` and `data()` are `const`. Is the null termination correctly
handled here?
--
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.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> TcrConnection::readMessage should not be explicitly allocating memory
> ---------------------------------------------------------------------
>
> Key: GEODE-10098
> URL: https://issues.apache.org/jira/browse/GEODE-10098
> Project: Geode
> Issue Type: Improvement
> Components: native client
> Reporter: Blake Bender
> Priority: Major
> Labels: pull-request-available
>
> This method calls new to read an array of bytes, then returns it to the
> caller, whose responsibility is to delete it (what the heck???). Even
> better, the memory is deleted in a call to TcrMessage::setData, so not even
> in the same class. If this memory was a std::vector<int8_t>, we could
> probably take advantage of move semantics and maybe even improve performance
> a bit, in addition to avoiding potential leaks and weirdness...
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
