New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Porting Wevtapi.dll #692

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
3 participants
@sakamotodesu
Contributor

sakamotodesu commented Aug 22, 2016

Porting Wevtapi.dll
Porting Winevt.h

Porting Wevtapi.dll
Porting Winevt.h
@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Aug 23, 2016

Member

Thank you - this looks very good - the following I noticed while going over the code:

  • Please add a line to CHANGES.md summarizing the new features
  • Check if Pointer/PointerByReference definitions can be turned into direct String/WString Mappings. I noticed this while looking over EVT_RPC_LOGIN. The members look as if they could be directly bound.
  • For structures defined outside a library definition (here: Winevt.java) make sure, that structures containing special types (primary strings) use the appropriate mappes. Please see ab84488 for a sample where this was necessary.
Member

matthiasblaesing commented Aug 23, 2016

Thank you - this looks very good - the following I noticed while going over the code:

  • Please add a line to CHANGES.md summarizing the new features
  • Check if Pointer/PointerByReference definitions can be turned into direct String/WString Mappings. I noticed this while looking over EVT_RPC_LOGIN. The members look as if they could be directly bound.
  • For structures defined outside a library definition (here: Winevt.java) make sure, that structures containing special types (primary strings) use the appropriate mappes. Please see ab84488 for a sample where this was necessary.
add PR to CHANGES.md
move `EVT_VARIANT` and `EVT_PRC_LOGIN` to the outside
change Pointer to WSing in `EVT_RPC_LOGIN`
add TypeMapper to `EVT_VARIANT` and `EVT_PRC_LOGIN`
@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Aug 24, 2016

Member

I'm not sure whether I can fully follow your changes -

  1. There is no need to move the structures out of Winevt.java (they were correct there)
  2. Mapping to WString is only necessary if the mapping is different from the typemapper. See LMShare.java in the sample, the members are defined as LPWSTR in the .h files and mapped to Strings.

Thank you - Matthias

Member

matthiasblaesing commented Aug 24, 2016

I'm not sure whether I can fully follow your changes -

  1. There is no need to move the structures out of Winevt.java (they were correct there)
  2. Mapping to WString is only necessary if the mapping is different from the typemapper. See LMShare.java in the sample, the members are defined as LPWSTR in the .h files and mapped to Strings.

Thank you - Matthias

@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Aug 28, 2016

Member
  • The javadoc in Wevtapi#EvtCancel is broken (</li> and </ul> are the correct closing tags)
  • EVT_RPC_LOGIN has to use W32APITypeMapper.UNICODE as mapper, als the members are mapped as LPWSTR (the Default Mapper is correct if the type is declared as LPTSTR)
  • EVT_VARIANT_TYPE#EvtVarTypeString declares AnsiStringVal as target field, not StringVal
  • EVT_VARIANT_TYPE does not map the array fields - suggested: add a getArrField function to the enumeration - the Type value of EVT_VARIANT needs to be masked for WevtapiTest#useMemory this could look like:
    private void useMemory(Winevt.EVT_VARIANT evtVariant, Memory buff, int index) {
        evtVariant.use(buff.share(evtVariant.size() * index));
        evtVariant.read();
        int typeIdx = evtVariant.Type;

        boolean isArray = (typeIdx & EVT_VARIANT_TYPE_ARRAY) == EVT_VARIANT_TYPE_ARRAY;
        int baseTypeIdx = typeIdx & EVT_VARIANT_TYPE_MASK;

        System.out.println("===== " + baseTypeIdx + " ======");

        Winevt.EVT_VARIANT_TYPE type = Winevt.EVT_VARIANT_TYPE.values()[baseTypeIdx];

        evtVariant.field1.use(buff.share(evtVariant.size() * index));
        evtVariant.field1.readField(isArray ? type.getArrField() : type.getField());
    }
  • I was not able to access the Event/EventData/Data fields in teestReadEvents. The mappings of the array fields in EVT_VARIANT look wrong (th StringVal mapping looks equally wrong).
  • This looks suspicious: assertThat("Provider Name", evtVariant.field1.AnsiStringVal.getWideString(0), is("testSource"));. The native string field is accessed and unicode is read from it. This is luck (or more precisely a specialty of UNIONs) that this works.

From my perspective the EVT_VARIANT looks to be the most problematic field and needs most attention.

Member

matthiasblaesing commented Aug 28, 2016

  • The javadoc in Wevtapi#EvtCancel is broken (</li> and </ul> are the correct closing tags)
  • EVT_RPC_LOGIN has to use W32APITypeMapper.UNICODE as mapper, als the members are mapped as LPWSTR (the Default Mapper is correct if the type is declared as LPTSTR)
  • EVT_VARIANT_TYPE#EvtVarTypeString declares AnsiStringVal as target field, not StringVal
  • EVT_VARIANT_TYPE does not map the array fields - suggested: add a getArrField function to the enumeration - the Type value of EVT_VARIANT needs to be masked for WevtapiTest#useMemory this could look like:
    private void useMemory(Winevt.EVT_VARIANT evtVariant, Memory buff, int index) {
        evtVariant.use(buff.share(evtVariant.size() * index));
        evtVariant.read();
        int typeIdx = evtVariant.Type;

        boolean isArray = (typeIdx & EVT_VARIANT_TYPE_ARRAY) == EVT_VARIANT_TYPE_ARRAY;
        int baseTypeIdx = typeIdx & EVT_VARIANT_TYPE_MASK;

        System.out.println("===== " + baseTypeIdx + " ======");

        Winevt.EVT_VARIANT_TYPE type = Winevt.EVT_VARIANT_TYPE.values()[baseTypeIdx];

        evtVariant.field1.use(buff.share(evtVariant.size() * index));
        evtVariant.field1.readField(isArray ? type.getArrField() : type.getField());
    }
  • I was not able to access the Event/EventData/Data fields in teestReadEvents. The mappings of the array fields in EVT_VARIANT look wrong (th StringVal mapping looks equally wrong).
  • This looks suspicious: assertThat("Provider Name", evtVariant.field1.AnsiStringVal.getWideString(0), is("testSource"));. The native string field is accessed and unicode is read from it. This is luck (or more precisely a specialty of UNIONs) that this works.

From my perspective the EVT_VARIANT looks to be the most problematic field and needs most attention.

sakamotodesu added some commits Aug 31, 2016

repair javadoc
add getArrField() function
change fields of `EvtVarTypeString` , `EvtVarTypeSByte`, `EvtVarTypeXml`
@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Sep 5, 2016

Member

You need to revisit this. As already written EVT_VARIANT is still a problem. The mapping to PointerByReference is wrong. The values are Pointers and you also use them as such:

 Pointer[] evtVariant.field1.StringArr.getPointer().getPointerArray(0);

getPointer retrieves the value of the field itself. If you use PointerByReference you are interested in the target (getValue()). As said - this is wrong.

I also looked at the XML mappings - these look strange and out of place (XML is mapped to unicode string/unicode strings).

I sat down and used a different approach (read the value directly from memory and not let JNA handle the mapping). The union definition is reduced to a placeholder - this works because if I'm not mistaken this structure is only ever read and not written.

Please have a look at it.

Member

matthiasblaesing commented Sep 5, 2016

You need to revisit this. As already written EVT_VARIANT is still a problem. The mapping to PointerByReference is wrong. The values are Pointers and you also use them as such:

 Pointer[] evtVariant.field1.StringArr.getPointer().getPointerArray(0);

getPointer retrieves the value of the field itself. If you use PointerByReference you are interested in the target (getValue()). As said - this is wrong.

I also looked at the XML mappings - these look strange and out of place (XML is mapped to unicode string/unicode strings).

I sat down and used a different approach (read the value directly from memory and not let JNA handle the mapping). The union definition is reduced to a placeholder - this works because if I'm not mistaken this structure is only ever read and not written.

Please have a look at it.

Show outdated Hide outdated contrib/platform/test/com/sun/jna/platform/win32/WevtapiTest.java
useMemory(evtVariant, buff, 1);
assertThat("EventRecordID", evtVariant.field1.UInt64Val, is((long) arrayIndex * eventArraySize + i + 1));
useMemory(evtVariant, buff, 2);
assertThat("EventID", evtVariant.field1.UInt64Val, is((long) arrayIndex * eventArraySize + i + 1));

This comment has been minimized.

@matthiasblaesing

matthiasblaesing Sep 11, 2016

Member

Why does this even work? EventId is in the range > 5000 in your test events. If I'm not mistaken, your reading the wrong field here and just because you read the field directly before (with the correct value).

@matthiasblaesing

matthiasblaesing Sep 11, 2016

Member

Why does this even work? EventId is in the range > 5000 in your test events. If I'm not mistaken, your reading the wrong field here and just because you read the field directly before (with the correct value).

@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Sep 11, 2016

Member

Please have a look at this:

matthiasblaesing@82e2975

It would have been good if you'd have indicated, that I did not provide the reference I was referring to. In the referenced commit I broadend the testbase base moved the logic for field selection into the EVT_VARIANT itself, releasing the user from the necessity to map type to correct field. (I also fixed the unittest I commented above).

Please note: If you implement a change - please give a reasoning why you think, that your change fixes the raised issues or gives reasons, that the presented solution is correct. For me this conversation is to onesided.

Member

matthiasblaesing commented Sep 11, 2016

Please have a look at this:

matthiasblaesing@82e2975

It would have been good if you'd have indicated, that I did not provide the reference I was referring to. In the referenced commit I broadend the testbase base moved the logic for field selection into the EVT_VARIANT itself, releasing the user from the necessity to map type to correct field. (I also fixed the unittest I commented above).

Please note: If you implement a change - please give a reasoning why you think, that your change fixes the raised issues or gives reasons, that the presented solution is correct. For me this conversation is to onesided.

@sakamotodesu

This comment has been minimized.

Show comment
Hide comment
@sakamotodesu

sakamotodesu Sep 14, 2016

Contributor

Thank you for reviewing. and I apologize that I did not write the comments necessary.(Because I'm not good at English.)

From the next time, I will write the more comments.

Contributor

sakamotodesu commented Sep 14, 2016

Thank you for reviewing. and I apologize that I did not write the comments necessary.(Because I'm not good at English.)

From the next time, I will write the more comments.

@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Sep 19, 2016

Member

@sakamotodesu no problem! Its good you stay with this. I still want to ask to look at the commit I referenced. The approach approaches the problem differently. While you try to bind the structure as literal as possible, it makes it easy to read the wrong field (the unittest demostrated that).

My approach takes a different route: It assumes the inner union (field1_union) just exists to block the correct size and have a pointer to access the data itself. The whole EVT_VARIANT structure looks like a poor mans object, with the object type declared by the Type member and the object data held in field1. No if I consider field1 as internal structure, that I don't want to have to deal with explizitly, I can implement a setter in the EVT_VARIANT structure, that decouples the java usage from the physical data.

For now I have two other PRs, that need attention first, so my responses might be slow the next few days.

Member

matthiasblaesing commented Sep 19, 2016

@sakamotodesu no problem! Its good you stay with this. I still want to ask to look at the commit I referenced. The approach approaches the problem differently. While you try to bind the structure as literal as possible, it makes it easy to read the wrong field (the unittest demostrated that).

My approach takes a different route: It assumes the inner union (field1_union) just exists to block the correct size and have a pointer to access the data itself. The whole EVT_VARIANT structure looks like a poor mans object, with the object type declared by the Type member and the object data held in field1. No if I consider field1 as internal structure, that I don't want to have to deal with explizitly, I can implement a setter in the EVT_VARIANT structure, that decouples the java usage from the physical data.

For now I have two other PRs, that need attention first, so my responses might be slow the next few days.

@sakamotodesu

This comment has been minimized.

Show comment
Hide comment
@sakamotodesu

sakamotodesu Sep 21, 2016

Contributor

matthiasblaesing@82e2975

Fantastic!
I was able to finally understand your approach.

We have got to consider about:

The costs loading Native

If possible, I want to reduce the number of loading native memory data into heap because that function is heavy. In your aproach(field1_union#read()), all fierds of field1_union are loaded from native memory. But if you use field1_union#readTypes("FieldName"), loading is once.

Previously when I implement consuming event logs, EVT_VARIANT is used many times and this loading costs was a bottleneck.

Invalid Memory Access

Is there possibility to occur the "Invalid Memory Access" error ? When I implement, at first I used field1_union#read(), so I got that error. I think that the reason is probably mismatch of native memory data type and one of the fields type.

Contributor

sakamotodesu commented Sep 21, 2016

matthiasblaesing@82e2975

Fantastic!
I was able to finally understand your approach.

We have got to consider about:

The costs loading Native

If possible, I want to reduce the number of loading native memory data into heap because that function is heavy. In your aproach(field1_union#read()), all fierds of field1_union are loaded from native memory. But if you use field1_union#readTypes("FieldName"), loading is once.

Previously when I implement consuming event logs, EVT_VARIANT is used many times and this loading costs was a bottleneck.

Invalid Memory Access

Is there possibility to occur the "Invalid Memory Access" error ? When I implement, at first I used field1_union#read(), so I got that error. I think that the reason is probably mismatch of native memory data type and one of the fields type.

@twall

This comment has been minimized.

Show comment
Hide comment
@twall

twall Sep 21, 2016

Contributor

Generally, unions will avoid reading non-primitive types if the union type
has not been set, but will attempt to read as many primitive values as it
can on a read() call.

On Wed, Sep 21, 2016 at 1:46 PM, Minoru Sakamoto notifications@github.com
wrote:

matthiasblaesing/jna@82e2975
matthiasblaesing@82e2975
Fantastic!
I was able to finally understand your approach.

We have got to consider about:
The costs loading Native

If possible, I want to reduce the number of loading native memory data
into heap because that function is heavy. In your
aproach(field1_union#read()), all fierds of field1_union are loaded from
native memory. But if you use field1_union#readTypes("FieldName"),
loading is once.

Previously when I implement consuming event logs, EVT_VARIANT is used many
times and this loading costs was a bottleneck.
Invalid Memory Access

Is there possibility to occur the "Invalid Memory Access" error ? When I
implement, at first I used field1_union#read(), so I got that error. I
think that the reason is probably mismatch of native memory data type and
one of the fields type.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#692 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrUr4QOr5ZNgZ5ubax9r8gf_3ZcSHks5qsW1dgaJpZM4JpsN8
.

Contributor

twall commented Sep 21, 2016

Generally, unions will avoid reading non-primitive types if the union type
has not been set, but will attempt to read as many primitive values as it
can on a read() call.

On Wed, Sep 21, 2016 at 1:46 PM, Minoru Sakamoto notifications@github.com
wrote:

matthiasblaesing/jna@82e2975
matthiasblaesing@82e2975
Fantastic!
I was able to finally understand your approach.

We have got to consider about:
The costs loading Native

If possible, I want to reduce the number of loading native memory data
into heap because that function is heavy. In your
aproach(field1_union#read()), all fierds of field1_union are loaded from
native memory. But if you use field1_union#readTypes("FieldName"),
loading is once.

Previously when I implement consuming event logs, EVT_VARIANT is used many
times and this loading costs was a bottleneck.
Invalid Memory Access

Is there possibility to occur the "Invalid Memory Access" error ? When I
implement, at first I used field1_union#read(), so I got that error. I
think that the reason is probably mismatch of native memory data type and
one of the fields type.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#692 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrUr4QOr5ZNgZ5ubax9r8gf_3ZcSHks5qsW1dgaJpZM4JpsN8
.

@twall

This comment has been minimized.

Show comment
Hide comment
@twall

twall Sep 21, 2016

Contributor

There are several ways you can avoid automatic read of struct/union when
initializing from native memory. If you never want to read
automatically, call Structure.setAutoRead(false). Alternatively you can
tweak the pointer-based constructor or Structure.read() to do what you want.

A good default pattern for unions is this:

public void read() {
    this.type_field = this.readField("type_field");
    this.setType(type_based_on_type_field);
    super.read();
}

On Wed, Sep 21, 2016 at 2:12 PM, Timothy Wall twalljava@java.net wrote:

Generally, unions will avoid reading non-primitive types if the union type
has not been set, but will attempt to read as many primitive values as it
can on a read() call.

On Wed, Sep 21, 2016 at 1:46 PM, Minoru Sakamoto <notifications@github.com

wrote:

matthiasblaesing/jna@82e2975
matthiasblaesing@82e2975
Fantastic!
I was able to finally understand your approach.

We have got to consider about:
The costs loading Native

If possible, I want to reduce the number of loading native memory data
into heap because that function is heavy. In your
aproach(field1_union#read()), all fierds of field1_union are loaded from
native memory. But if you use field1_union#readTypes("FieldName"),
loading is once.

Previously when I implement consuming event logs, EVT_VARIANT is used
many times and this loading costs was a bottleneck.
Invalid Memory Access

Is there possibility to occur the "Invalid Memory Access" error ? When I
implement, at first I used field1_union#read(), so I got that error. I
think that the reason is probably mismatch of native memory data type and
one of the fields type.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#692 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrUr4QOr5ZNgZ5ubax9r8gf_3ZcSHks5qsW1dgaJpZM4JpsN8
.

Contributor

twall commented Sep 21, 2016

There are several ways you can avoid automatic read of struct/union when
initializing from native memory. If you never want to read
automatically, call Structure.setAutoRead(false). Alternatively you can
tweak the pointer-based constructor or Structure.read() to do what you want.

A good default pattern for unions is this:

public void read() {
    this.type_field = this.readField("type_field");
    this.setType(type_based_on_type_field);
    super.read();
}

On Wed, Sep 21, 2016 at 2:12 PM, Timothy Wall twalljava@java.net wrote:

Generally, unions will avoid reading non-primitive types if the union type
has not been set, but will attempt to read as many primitive values as it
can on a read() call.

On Wed, Sep 21, 2016 at 1:46 PM, Minoru Sakamoto <notifications@github.com

wrote:

matthiasblaesing/jna@82e2975
matthiasblaesing@82e2975
Fantastic!
I was able to finally understand your approach.

We have got to consider about:
The costs loading Native

If possible, I want to reduce the number of loading native memory data
into heap because that function is heavy. In your
aproach(field1_union#read()), all fierds of field1_union are loaded from
native memory. But if you use field1_union#readTypes("FieldName"),
loading is once.

Previously when I implement consuming event logs, EVT_VARIANT is used
many times and this loading costs was a bottleneck.
Invalid Memory Access

Is there possibility to occur the "Invalid Memory Access" error ? When I
implement, at first I used field1_union#read(), so I got that error. I
think that the reason is probably mismatch of native memory data type and
one of the fields type.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#692 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrUr4QOr5ZNgZ5ubax9r8gf_3ZcSHks5qsW1dgaJpZM4JpsN8
.

@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Sep 29, 2016

Member

So I had a very short look at the performance side and both approaches show similar timings.

My main problem: EVT_VARIANT feels awkward - I have to check type and read the correct field, its quite possible to get to the right field. I don't like to use that interface. See also the differences in useMemory (the complex read operation).

My addition also covered more types for the EVT_VARIANT:

  • Event/EventData/Data (String array)
  • Event/System/TimeCreated/@SystemTime (FILETIME)
  • Event/EventData/Binary (byte array)
  • Event/System/Correlation (null)
  • Event/System/Security/@userid (PSID)
  • Event/System/Provider/@Guid (GUID)

Have a look at the renamed/added event files.

Member

matthiasblaesing commented Sep 29, 2016

So I had a very short look at the performance side and both approaches show similar timings.

My main problem: EVT_VARIANT feels awkward - I have to check type and read the correct field, its quite possible to get to the right field. I don't like to use that interface. See also the differences in useMemory (the complex read operation).

My addition also covered more types for the EVT_VARIANT:

  • Event/EventData/Data (String array)
  • Event/System/TimeCreated/@SystemTime (FILETIME)
  • Event/EventData/Binary (byte array)
  • Event/System/Correlation (null)
  • Event/System/Security/@userid (PSID)
  • Event/System/Provider/@Guid (GUID)

Have a look at the renamed/added event files.

@sakamotodesu

This comment has been minimized.

Show comment
Hide comment
@sakamotodesu

sakamotodesu Oct 6, 2016

Contributor

I measured the reading speed of 1GB evtx file, there was no difference.
I will merge @matthiasblaesing 's EVT_VARIANT#getValue() .

Contributor

sakamotodesu commented Oct 6, 2016

I measured the reading speed of 1GB evtx file, there was no difference.
I will merge @matthiasblaesing 's EVT_VARIANT#getValue() .

@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Oct 10, 2016

Member

@sakamotodesu Looks good so far. Thanks for staying with it!

Some optimizations are possible:

  • Introduce EVT_HANDLE as a subclass of HANDLE (seperate EVT_HANDLE from HANDLE)
  • EvtNext: Replace "Pointer EventArray" with EVT_HANDLE[]
  • EvtCreateRenderContext: Replace "Pointer ValuePaths" with String[]
  • EvtFormatMessage: Replace "Pointer Values" with EVT_VARIANT[]
  • EvtSetChannelConfigProperty: Replace "Pointer PropertyValue" with EVT_VARIANT

See these here: matthiasblaesing@18d3483

I looked through the methods and only found one, that takes a EVT_VARIANT as input: EvtSetChannelConfigProperty. So I implemented basic setter support for the important values (these cover all settable property datatypes). Please have a look at this:

matthiasblaesing@176b2f3

Member

matthiasblaesing commented Oct 10, 2016

@sakamotodesu Looks good so far. Thanks for staying with it!

Some optimizations are possible:

  • Introduce EVT_HANDLE as a subclass of HANDLE (seperate EVT_HANDLE from HANDLE)
  • EvtNext: Replace "Pointer EventArray" with EVT_HANDLE[]
  • EvtCreateRenderContext: Replace "Pointer ValuePaths" with String[]
  • EvtFormatMessage: Replace "Pointer Values" with EVT_VARIANT[]
  • EvtSetChannelConfigProperty: Replace "Pointer PropertyValue" with EVT_VARIANT

See these here: matthiasblaesing@18d3483

I looked through the methods and only found one, that takes a EVT_VARIANT as input: EvtSetChannelConfigProperty. So I implemented basic setter support for the important values (these cover all settable property datatypes). Please have a look at this:

matthiasblaesing@176b2f3

optimizations:
Introduce EVT_HANDLE as a subclass of HANDLE (seperate EVT_HANDLE from HANDLE)
EvtNext: Replace "Pointer EventArray" with EVT_HANDLE[]
EvtCreateRenderContext: Replace "Pointer ValuePaths" with String[]
EvtFormatMessage: Replace "Pointer Values" with EVT_VARIANT[]
EvtSetChannelConfigProperty: Replace "Pointer PropertyValue" with EVT_VARIANT
@sakamotodesu

This comment has been minimized.

Show comment
Hide comment
@sakamotodesu

sakamotodesu Oct 20, 2016

Contributor

matthiasblaesing/jna@176b2f3

@matthiasblaesing Great! I didn't know that API can determine used memory size. I will make more util methods for other APIs.

I want to get Windows Error Code from util methods for error handling.
Because Windows Error code has many information, and there is a case which the process branches.
Do you have any suggestions?

Contributor

sakamotodesu commented Oct 20, 2016

matthiasblaesing/jna@176b2f3

@matthiasblaesing Great! I didn't know that API can determine used memory size. I will make more util methods for other APIs.

I want to get Windows Error Code from util methods for error handling.
Because Windows Error code has many information, and there is a case which the process branches.
Do you have any suggestions?

@sakamotodesu

This comment has been minimized.

Show comment
Hide comment
Contributor

sakamotodesu commented Oct 21, 2016

adding utilities:
  EvtGetExtendedStatus
  EvtRender
  EvtFormatMessage
  EvtGetChannelConfigProperty
  EvtNextPublisherId
  EvtGetPublisherMetadataProperty
@sakamotodesu

This comment has been minimized.

Show comment
Hide comment
@sakamotodesu

sakamotodesu Nov 1, 2016

Contributor

Since I encounted 0xc0000374 error when allocating memory size was few, I made "safeAllocate()".
I didn't decide which caused by this error Windows or JNA.

steps to reproduce

  1. Remove "safeAllocate()", allocating by IntByReference
  2. Run WevtapiTest on IntelliJ IDEA
  3. If you run that test sometime, maybe you get 0xc0000374

environment

  • Windows 10 version 1607 OS build 14393.351, Japanese Edition
  • IntelliJ IDEA 2016.2.5
  • JDK1.8.0u101
Contributor

sakamotodesu commented Nov 1, 2016

Since I encounted 0xc0000374 error when allocating memory size was few, I made "safeAllocate()".
I didn't decide which caused by this error Windows or JNA.

steps to reproduce

  1. Remove "safeAllocate()", allocating by IntByReference
  2. Run WevtapiTest on IntelliJ IDEA
  3. If you run that test sometime, maybe you get 0xc0000374

environment

  • Windows 10 version 1607 OS build 14393.351, Japanese Edition
  • IntelliJ IDEA 2016.2.5
  • JDK1.8.0u101
@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Nov 2, 2016

Member

You have an error in your logic. Have a look at WevtapiTest#EvtNextPublisherId this fails for me reproducible (when regular memory allocation is used) and I can also tell you why: Your overflowing the heap and in fact this matches the error you receive.

Let's have a look at your implementation:

        // This is com.sun.jna.platform.win32.WevtapiUtil#EvtNextPublisherId (line 194)
        IntByReference publisherIdBufferUsed = new IntByReference();
        boolean result = Wevtapi.INSTANCE.EvtNextPublisherId(publisherEnum, 0, null, publisherIdBufferUsed);
        int errorCode = Kernel32.INSTANCE.GetLastError();
        if ((!result) && errorCode != Kernel32.ERROR_INSUFFICIENT_BUFFER) {
            throw new Win32Exception(errorCode);
        }

        //  -----------------  TOO SMALL BUFFER ALLOCATED ------------
        Memory publisherIdBuffer = safeAllocate(publisherIdBufferUsed.getValue());
        result = Wevtapi.INSTANCE.EvtNextPublisherId(publisherEnum, (int) publisherIdBuffer.size(), publisherIdBuffer, publisherIdBufferUsed);
        if (!result) {
            throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
        }
        return publisherIdBuffer;

The fourth parameter of EvtNextPublisherId is documented as:

PublisherIdBufferUsed [out]

The size, in characters, of the caller-allocated buffer that the function used or
the required buffer size if the function fails with ERROR_INSUFFICIENT_BUFFER.

The buffer holds LPWSTR (wide string), so the returned number is half the size you'll need in bytes (wide string are UCS-2 so 2 bytes). On each call to EvtNextPublishedId overwrites some memory, that it should not.

Your work around works because all allocations I saw are below 1024 and so your safeAllocate function masked the problem (you'd have seen problems when the character count surpassed 512 characters).

In addition please also see if you want to rereview:

matthiasblaesing@176b2f3

That commit held more than the introduction of a util method (in fact that was collateral material) - there is a whole implementation of write support for EVT_VARIANT with accompied unittest.

One final request: If you think about copying a commit, think about merging it into your branch.

Member

matthiasblaesing commented Nov 2, 2016

You have an error in your logic. Have a look at WevtapiTest#EvtNextPublisherId this fails for me reproducible (when regular memory allocation is used) and I can also tell you why: Your overflowing the heap and in fact this matches the error you receive.

Let's have a look at your implementation:

        // This is com.sun.jna.platform.win32.WevtapiUtil#EvtNextPublisherId (line 194)
        IntByReference publisherIdBufferUsed = new IntByReference();
        boolean result = Wevtapi.INSTANCE.EvtNextPublisherId(publisherEnum, 0, null, publisherIdBufferUsed);
        int errorCode = Kernel32.INSTANCE.GetLastError();
        if ((!result) && errorCode != Kernel32.ERROR_INSUFFICIENT_BUFFER) {
            throw new Win32Exception(errorCode);
        }

        //  -----------------  TOO SMALL BUFFER ALLOCATED ------------
        Memory publisherIdBuffer = safeAllocate(publisherIdBufferUsed.getValue());
        result = Wevtapi.INSTANCE.EvtNextPublisherId(publisherEnum, (int) publisherIdBuffer.size(), publisherIdBuffer, publisherIdBufferUsed);
        if (!result) {
            throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
        }
        return publisherIdBuffer;

The fourth parameter of EvtNextPublisherId is documented as:

PublisherIdBufferUsed [out]

The size, in characters, of the caller-allocated buffer that the function used or
the required buffer size if the function fails with ERROR_INSUFFICIENT_BUFFER.

The buffer holds LPWSTR (wide string), so the returned number is half the size you'll need in bytes (wide string are UCS-2 so 2 bytes). On each call to EvtNextPublishedId overwrites some memory, that it should not.

Your work around works because all allocations I saw are below 1024 and so your safeAllocate function masked the problem (you'd have seen problems when the character count surpassed 512 characters).

In addition please also see if you want to rereview:

matthiasblaesing@176b2f3

That commit held more than the introduction of a util method (in fact that was collateral material) - there is a whole implementation of write support for EVT_VARIANT with accompied unittest.

One final request: If you think about copying a commit, think about merging it into your branch.

@sakamotodesu

This comment has been minimized.

Show comment
Hide comment
@sakamotodesu

sakamotodesu Nov 3, 2016

Contributor

I failed to merge... so sorry.
This PR has a lot of commits, so I will make new PR.

Contributor

sakamotodesu commented Nov 3, 2016

I failed to merge... so sorry.
This PR has a lot of commits, so I will make new PR.

@matthiasblaesing

This comment has been minimized.

Show comment
Hide comment
@matthiasblaesing

matthiasblaesing Nov 3, 2016

Member

I'll have a look at the new PR thanks so far. I'm closing this PR then.

Member

matthiasblaesing commented Nov 3, 2016

I'll have a look at the new PR thanks so far. I'm closing this PR then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment