Improve property parsing #41
Conversation
The last property parsed was not added to the cache and the property index was not advanced. This resulted in extra work during parsing and the same property being present multiple times in the cache.
Strings at the end of a event buffer may not contain a null terminator.
|
@aydany thanks for the pull request! I'm reviewing now. Also cc @zacbrown Here's the TraceEventRawReaders reference: https://github.com/Microsoft/perfview/blob/master/src/TraceEvent/TraceEvent.cs#L3886 |
|
@aydany would you be willing to contribute a unit test that covers the non-null-terminated case so we have test coverage on this new code? |
| // The property was found, return it | ||
| if (name == pName) { | ||
| // advance the index since we've already processed this property | ||
| ++i; |
There was a problem hiding this comment.
is this line necessary? if we're about to return propInfo then i will go out of scope so we don't need to advance it?
There was a problem hiding this comment.
Note that i is defined as a reference to lastPropertyIndex_` at the start of the for loop.
There was a problem hiding this comment.
You're right, I missed the ampersand. Thanks!
|
@swannman I'll take a look at adding a test tomorrow. Thanks for the review! |
krabs/krabs/size_provider.hpp
Outdated
| // size of unicode string is length + 1 (null byte) * sizeof(wchar) | ||
| auto chars = (ULONG)wcslen((const wchar_t*)propertyStart) + 1; | ||
| propertyLength = chars * sizeof(wchar_t); | ||
| const wchar_t* p = (const wchar_t*)propertyStart; |
There was a problem hiding this comment.
Stylistically - prefer auto here.
There was a problem hiding this comment.
ditto line below as well
krabs/krabs/size_provider.hpp
Outdated
| } | ||
| else if (propertyInfo.nonStructType.InType == TDH_INTYPE_ANSISTRING) | ||
| { | ||
| // size of ansi string is length + 1 (null byte) | ||
| propertyLength = (ULONG)strlen((const char*)propertyStart) + 1; | ||
| const char* p = (const char*)propertyStart; |
There was a problem hiding this comment.
ditto line below as well
|
@aydany it looks like there are a few minor changes necessary before we can merge this in. Are you still willing to make those? |
Modified the record_generator to trim the generated record (optionally) if the last property is a string. Include a test for UNICODE strings, but do not have an ETW event for to test the ANSI string case. The included test results in error, when the size check changes are reverted.
|
Nice! @zacbrown any concerns? |
|
I apologize for taking way too long to get back to this. I addressed the comment about style and added a unit test to cover the code changes. The test will fail when run against master, but passes here. Please note that the test covers only the UNICODE string case. To cover the ANSI string case we need a valid ETW event that has an ANSI string as its last property. I happened to encounter the UNICODE case accidentally and I do not have an example that can be used for the ANSI case at this time. |
This set introduces two improvements:
The properties cache in the parser could store multiple copies of the same property
This situation could occur because the last property parsed was added to the cache and returned but the buffer pointer and property index were not incremented. This is now fixed: every parsed property can exist in the cache just once and there is no duplicate property parsing.
String values (ANSI and UNICODE) that belong to the last property in an event record are not guaranteed to be null-terminated. The code assumes that every string is null-terminated, which causes strlen and wcslen to potentially access invalid memory looking for a null terminator. If the memory access succeeds, the parser will throw an exception as the length of the string property will be too large. These error conditions were exposed when processing events from http.sys. I modeled the fix after the way Microsoft.Diagnostics.Tracing.TraceEventRawReaders deals with strings. All unit tests still pass and the events from http.sys are now processed without errors.