Fix bug in VARDESC (#1644) #1645
Conversation
|
I have amended the commit with a change log entry and with spaces instead of tabs :) |
stmuecke
force-pushed
the
master
branch
2 times, most recently
from
7206769 to
bfc7610
Compare
|
Is just discovered the same problem in TYPEDESC. Do you want me to update this pull request or open another one? |
|
@stmuecke go ahead and update this PR. |
|
Should I force-push it as previously, or what's the proper way to do it? (Sorry, I am new to collaboration on GitHub.) |
|
@stmuecke to my experience there is no general rule. But given the size of the changes I say, just update in place. |
stmuecke
force-pushed
the
master
branch
2 times, most recently
from
2c7161a to
933fc64
Compare
Most GitHub repos use "squash and merge" to make PRs a single commit anyway. Squashing it yourself gives you a bit better control over the commit messages, although I also think most maintainers are happy enough with any contribution they're not going to be overly picky about it. Separate commits are useful in the review phase when there are more complex changes but when the changes are confined to a single class per "commit" there's not really any need. |
| case Variant.VT_CARRAY : | ||
| _typedesc.setType("lpadesc"); | ||
| break; | ||
| case Variant.VT_USERDEFINED : |
There was a problem hiding this comment.
Should this also be the default? the hreftype is safe to read in any situation as it is just a plain DWORD and thus won't cause further reads. For the VARDESC this is implicitly done and I think it would be right here too.
There was a problem hiding this comment.
It may be safe to read, but it sets the union's type to a value that doesn't apply. I consider this to be incorrect. But maybe my idea of unions is wrong.
There was a problem hiding this comment.
The problem I see is, that if you don't set the type, it might be already be set to one of the other types, which would result in an invalid read. It is the responsibility of the calling code to only read the value of the union if it makes sense, but we need to protect from invalid reads.
|
When you update, would you mind rebasing your change onto current master? The merge of #1642 created a merge conflict as both changes touch the same region in |
|
Default case added and rebased onto master. |
|
Could you please check this error reported by appveyor (runs Windows Unittests): The error is reproducible locally. |
|
I can reproduce the error, but it is evasive. I have written a TypeLibDumper class which causes the same invalid memory access as the unit test, but only under certain circumstances. The invalid access happens at type index 21 (IShellFolderViewDual3), function index 16 (CurrentViewMode). When I skip functions 0-15, the invalid memory access doesn't happen. It also doesn't happen when I dump the whole typelib, only when I skip types 0 to 20. Thus, the bug should be somewhere else. I have currently no idea how to track it down. Any suggestions? |
|
Here's a minimal reproducible example: The bug will not exhibit if one of the following changes is made:
|
|
The error in the unit test can also be reproduced with JNA 5.15.0 and therefore has nothing to do with the changes in this PR. I suggest you accept and close this PR and issue #1644, and we open a new issue for the unit test error. |
I concur. Will let @matthiasblaesing make the final determination |
|
Where does this:
come from? I never saw errors in So merging, would break currently working code. This does not mean, that I think this PR in itself introduces the issue but it changes the problem front. |
I agree we shouldn't merge code that breaks CI (even if it just "fixes" a problem that hid a bug elsewhere). I missed that AppVeyor isn't included in the "checks" on this PR, which all passed and I didn't realize didn't include Windows. Do we know why that is? Also see #1596 which I had to visit to hunt down your appveyor runs. As far as diagnosing a possible cause:
|
|
I have been trying to get closer to the root issue, and my impression so far is that something is going wrong on the native side. Using my typelib dumper, I compared the data I get with and without the error. There is a discrepancy in the data returned by GetFuncDesc even before the error occurs. When I start my dumping loop at type index 0, everything is okay. When I start at type index 20, the functions at indexes 11 and 15 contain different data in Here's a snippet from the console output, with "BEFORE" and "AFTER" showing the raw memory backing the FUNCDESC structures. The hypens in the memory dumps represent padding bytes. (BTW, it would be nice to be able to access StructField; I used reflection to access the memory layout information.) Here's the output when the error occurs shortly afterwards: Notice how in the first snippet the pointer values for FUNCDESC are both In the first snippet the FUNCDESC contains In the first snippet we get a I'll continue to dig around. Any hints are appreciated. |
|
Can you explain how this differing output is generated and what it means that the - ELEMDESC {tdesc: #TYPEDESC {vt: VT_PTR, TYPEDESC {vt: VT_VARIANT } }
+ ELEMDESC {tdesc: TYPEDESC {vt: VT_UINT } |
|
I just print out the information in the structures. For VT_UINT there is no more interesting information, while for VT_PTR the nested _TYPEDESC contains a reference to another TYPEDESC which is printed inline. Ignore the hashtag before TYPEDESC. This was only temporary to make things easier to find. |
|
@matthiasblaesing Please run the following snippet on unmodified JNA 5.15.0 and compare the results with master. See how much better it works with the fixes in place. Run it a few times on master. There are occasional totally random errors. |
|
JNA 15.5.0 gives me 192 errors. JNA master plus fixes gives me 0 to 3 errors. |
|
@stmuecke I get your point, the situation is improved for some situations. BUT it comes with a hefty price, because code that worked before now blows up. Consider the situation reversed: You are using JNA 5.15.0, your code works. No you upgrade to 5.16.0, now you get a segfault. What would you say? |
No description provided.