fgerlits commented on a change in pull request #928:
URL: https://github.com/apache/nifi-minifi-cpp/pull/928#discussion_r508332309
##########
File path: extensions/windows-event-log/ConsumeWindowsEventLog.cpp
##########
@@ -514,30 +515,33 @@ void
ConsumeWindowsEventLog::substituteXMLPercentageItems(pugi::xml_document& do
bool ConsumeWindowsEventLog::createEventRender(EVT_HANDLE hEvent, EventRender&
eventRender) {
logger_->log_trace("Rendering an event");
- DWORD size = 0;
+ DWORD size = sizeof(WCHAR) * 4096;
+ WCHAR stackBuffer[4096];
+ using Deleter = utils::StackAwareDeleter<WCHAR, utils::FreeDeleter>;
+ std::unique_ptr<WCHAR, Deleter> buf{stackBuffer, Deleter{ stackBuffer }};
+
DWORD used = 0;
DWORD propertyCount = 0;
- EvtRender(NULL, hEvent, EvtRenderEventXml, size, 0, &used, &propertyCount);
- if (ERROR_INSUFFICIENT_BUFFER != GetLastError()) {
- LOG_LAST_ERROR(EvtRender);
- return false;
- }
-
- if (used > maxBufferSize_) {
- logger_->log_error("Dropping event because it couldn't be rendered within
%" PRIu64 " bytes.", maxBufferSize_);
- return false;
- }
-
- size = used;
- std::vector<wchar_t> buf(size / 2 + 1);
- if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, &buf[0], &used,
&propertyCount)) {
- LOG_LAST_ERROR(EvtRender);
- return false;
+ if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, buf.get(), &used,
&propertyCount)) {
+ if (ERROR_INSUFFICIENT_BUFFER != GetLastError()) {
+ LOG_LAST_ERROR(EvtRender);
+ return false;
+ }
+ if (used > maxBufferSize_) {
+ logger_->log_error("Dropping event because it couldn't be rendered
within %" PRIu64 " bytes.", maxBufferSize_);
+ return false;
+ }
+ size = used;
+ buf.reset((LPWSTR)malloc(size));
+ if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, buf.get(), &used,
&propertyCount)) {
+ LOG_LAST_ERROR(EvtRender);
+ return false;
+ }
}
logger_->log_debug("Event rendered with size %" PRIu32 ". Performing doc
traversing...", size);
Review comment:
if the first (on-stack) `EvtRender` is successful, then `size` has not
been set to `used` at this point, it is still `sizeof(WCHAR) * 4096`
```suggestion
logger_->log_debug("Event rendered with size %" PRIu32 ". Performing doc
traversing...", used);
```
##########
File path: extensions/windows-event-log/ConsumeWindowsEventLog.cpp
##########
@@ -514,30 +515,33 @@ void
ConsumeWindowsEventLog::substituteXMLPercentageItems(pugi::xml_document& do
bool ConsumeWindowsEventLog::createEventRender(EVT_HANDLE hEvent, EventRender&
eventRender) {
logger_->log_trace("Rendering an event");
- DWORD size = 0;
+ DWORD size = sizeof(WCHAR) * 4096;
+ WCHAR stackBuffer[4096];
+ using Deleter = utils::StackAwareDeleter<WCHAR, utils::FreeDeleter>;
+ std::unique_ptr<WCHAR, Deleter> buf{stackBuffer, Deleter{ stackBuffer }};
+
DWORD used = 0;
DWORD propertyCount = 0;
- EvtRender(NULL, hEvent, EvtRenderEventXml, size, 0, &used, &propertyCount);
- if (ERROR_INSUFFICIENT_BUFFER != GetLastError()) {
- LOG_LAST_ERROR(EvtRender);
- return false;
- }
-
- if (used > maxBufferSize_) {
- logger_->log_error("Dropping event because it couldn't be rendered within
%" PRIu64 " bytes.", maxBufferSize_);
- return false;
- }
-
- size = used;
- std::vector<wchar_t> buf(size / 2 + 1);
- if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, &buf[0], &used,
&propertyCount)) {
- LOG_LAST_ERROR(EvtRender);
- return false;
+ if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, buf.get(), &used,
&propertyCount)) {
+ if (ERROR_INSUFFICIENT_BUFFER != GetLastError()) {
+ LOG_LAST_ERROR(EvtRender);
+ return false;
+ }
+ if (used > maxBufferSize_) {
+ logger_->log_error("Dropping event because it couldn't be rendered
within %" PRIu64 " bytes.", maxBufferSize_);
+ return false;
+ }
+ size = used;
+ buf.reset((LPWSTR)malloc(size));
Review comment:
I think we should check if the `malloc` succeeded (also in
`getEventData()`)
##########
File path: extensions/windows-event-log/wel/WindowsEventLog.cpp
##########
@@ -129,34 +133,30 @@ std::string
WindowsEventLogMetadataImpl::getEventData(EVT_FORMAT_MESSAGE_FLAGS f
return event_data;
}
- if (!EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer, &string_buffer_used)) {
+
+ if (!EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer.get(), &string_buffer_used)) {
result = GetLastError();
if (ERROR_INSUFFICIENT_BUFFER == result) {
string_buffer_size = string_buffer_used;
- string_buffer = (LPWSTR) malloc(string_buffer_size * sizeof(WCHAR));
-
- if (string_buffer) {
+ string_buffer.reset((LPWSTR) malloc(string_buffer_size * sizeof(WCHAR)));
- if ((EvtFormatMessageKeyword == flags))
- string_buffer[string_buffer_size - 1] = L'\0';
-
- EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer, &string_buffer_used);
- if ((EvtFormatMessageKeyword == flags))
- string_buffer[string_buffer_used - 1] = L'\0';
- std::wstring str(string_buffer);
- event_data = std::string(str.begin(), str.end());
- free(string_buffer);
- }
+ EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer.get(), &string_buffer_used);
}
}
+ if ((EvtFormatMessageKeyword == flags))
+ string_buffer.get()[string_buffer_used - 1] = L'\0';
+ std::wstring str(string_buffer.get());
+ event_data = std::string(str.begin(), str.end());
return event_data;
}
std::string WindowsEventLogHandler::getEventMessage(EVT_HANDLE eventHandle)
const {
std::string returnValue;
- std::unique_ptr<WCHAR, utils::FreeDeleter> pBuffer;
- DWORD dwBufferSize = 0;
+ DWORD dwBufferSize = 4096;
Review comment:
here, too, I would rename this
##########
File path: extensions/windows-event-log/wel/WindowsEventLog.cpp
##########
@@ -118,8 +120,10 @@ void WindowsEventLogMetadataImpl::renderMetadata() {
}
std::string WindowsEventLogMetadataImpl::getEventData(EVT_FORMAT_MESSAGE_FLAGS
flags) const {
- LPWSTR string_buffer = NULL;
- DWORD string_buffer_size = 0;
+ DWORD string_buffer_size = 4096;
Review comment:
I don't know why Microsoft decided to call this `BufferSize` but store
the number of characters in it, but we could give it a better name, eg.
`num_chars_in_buffer`.
##########
File path: extensions/windows-event-log/wel/WindowsEventLog.cpp
##########
@@ -129,34 +133,30 @@ std::string
WindowsEventLogMetadataImpl::getEventData(EVT_FORMAT_MESSAGE_FLAGS f
return event_data;
}
- if (!EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer, &string_buffer_used)) {
+
+ if (!EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer.get(), &string_buffer_used)) {
result = GetLastError();
if (ERROR_INSUFFICIENT_BUFFER == result) {
string_buffer_size = string_buffer_used;
- string_buffer = (LPWSTR) malloc(string_buffer_size * sizeof(WCHAR));
-
- if (string_buffer) {
+ string_buffer.reset((LPWSTR) malloc(string_buffer_size * sizeof(WCHAR)));
- if ((EvtFormatMessageKeyword == flags))
- string_buffer[string_buffer_size - 1] = L'\0';
-
- EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer, &string_buffer_used);
- if ((EvtFormatMessageKeyword == flags))
- string_buffer[string_buffer_used - 1] = L'\0';
- std::wstring str(string_buffer);
- event_data = std::string(str.begin(), str.end());
- free(string_buffer);
- }
+ EvtFormatMessage(metadata_ptr_, event_ptr_, 0, 0, NULL, flags,
string_buffer_size, string_buffer.get(), &string_buffer_used);
}
}
+ if ((EvtFormatMessageKeyword == flags))
Review comment:
minor, and old code, but why do we have double `(())` 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.
For queries about this service, please contact Infrastructure at:
[email protected]
