Skip to content
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

Fix bug with recursive IP_ADDR_STRING ByReference #986

Merged
merged 3 commits into from
Jul 20, 2018

Conversation

dbwiddis
Copy link
Contributor

@dbwiddis dbwiddis commented Jul 8, 2018

While the GetNetworkParams testcase in #983 worked fine on my Windows7 machine, it created Invalid Access Errors on Win 10, probably related to a combination recursive pre-reading of ByReference structures in the constructor, and local thread caching. See JNA mailing list thread for details. Bottom line: the IP_ADDR_STRING linked list needs to be mapped as a Pointer and the linked list traversed manually.

@matthiasblaesing
Copy link
Member

I tested with JNA master and could not reproduce the issue with the unittests. I tested on Windows 10 with Oracle Java 8 (32+64 bit). Can you come up with a reproducer? I have a bad feeling merging this without being able to reproduce it.

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Jul 18, 2018

As mentioned on the mailing list. This rarely occurs on the first time, I have to repeatedly call it to generate the error. The following code reliably reproduces the error for me (this is exactly what's in the testcase, minus the various asserts.) The Exception occurs in the FIXED_INFO constructor at the read().

    public static void testGetNetworkParams() {
        IntByReference bufferSize = new IntByReference();
        IPHlpAPI.INSTANCE.GetNetworkParams(null, bufferSize);
        FIXED_INFO buffer = new FIXED_INFO(new Memory(bufferSize.getValue()));
        IPHlpAPI.INSTANCE.GetNetworkParams(buffer, bufferSize);

        // Check all DNS servers are valid IPs
        IPHlpAPI.IP_ADDR_STRING dns = buffer.DnsServerList;
        while (dns != null) {
            // Start with 16-char byte array
            String addr = new String(dns.IpAddress.String);
            // addr String will have length 16; trim off trailing null(s)
            int nullPos = addr.indexOf(0);
            if (nullPos != -1) {
                addr = addr.substring(0, nullPos);
            }
            dns = dns.Next;
        }
    }

    public static void main(String[] args) {
        for (int i = 1; i < 50; i++) {
            System.out.println("Attempt " + i);
            testGetNetworkParams();
        }
    }

Log... note it worked fine 5 times. (Other times it's done only 1, sometimes I've gotten over 40.)

Attempt 1
Attempt 2
Attempt 3
Attempt 4
Attempt 5
Exception in thread "main" java.lang.Error: Invalid memory access
at com.sun.jna.Native._getPointer(Native Method)
at com.sun.jna.Native.getPointer(Native.java:2138)
at com.sun.jna.Pointer.getPointer(Pointer.java:651)
at com.sun.jna.Pointer.getValue(Pointer.java:376)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.Structure.autoRead(Structure.java:2074)
at com.sun.jna.Structure.conditionalAutoRead(Structure.java:550)
at com.sun.jna.Structure.updateStructureByReference(Structure.java:678)
at com.sun.jna.Pointer.getValue(Pointer.java:376)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.Structure.autoRead(Structure.java:2074)
at com.sun.jna.Structure.conditionalAutoRead(Structure.java:550)
at com.sun.jna.Structure.updateStructureByReference(Structure.java:678)
at com.sun.jna.Pointer.getValue(Pointer.java:376)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.Structure.autoRead(Structure.java:2074)
at com.sun.jna.Structure.conditionalAutoRead(Structure.java:550)
at com.sun.jna.Structure.updateStructureByReference(Structure.java:678)
at com.sun.jna.Pointer.getValue(Pointer.java:376)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.Structure.autoRead(Structure.java:2074)
at com.sun.jna.Structure.conditionalAutoRead(Structure.java:550)
at com.sun.jna.Structure.updateStructureByReference(Structure.java:678)
at com.sun.jna.Pointer.getValue(Pointer.java:376)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.Pointer.getValue(Pointer.java:379)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.platform.win32.IPHlpAPI$FIXED_INFO.(IPHlpAPI.java:208)
at oshi.software.os.windows.WindowsNetworkParams.testGetNetworkParams(WindowsNetworkParams.java:229)
at oshi.software.os.windows.WindowsNetworkParams.main(WindowsNetworkParams.java:249)

Sometimes the trace is different. This may have to do with recursive calls as it traverses the linked list.

Attempt 1
Attempt 2
Attempt 3
Attempt 4
Attempt 5
Attempt 6
Attempt 7
Attempt 8
Attempt 9
Attempt 10
Exception in thread "main" java.lang.Error: Invalid memory access
at com.sun.jna.Native._getPointer(Native Method)
at com.sun.jna.Native.getPointer(Native.java:2138)
at com.sun.jna.Pointer.getPointer(Pointer.java:651)
at com.sun.jna.Pointer.getValue(Pointer.java:376)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.Structure.autoRead(Structure.java:2074)
at com.sun.jna.Structure.conditionalAutoRead(Structure.java:550)
at com.sun.jna.Structure.updateStructureByReference(Structure.java:678)
at com.sun.jna.Pointer.getValue(Pointer.java:376)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.Pointer.getValue(Pointer.java:379)
at com.sun.jna.Structure.readField(Structure.java:720)
at com.sun.jna.Structure.read(Structure.java:580)
at com.sun.jna.platform.win32.IPHlpAPI$FIXED_INFO.(IPHlpAPI.java:208)
at oshi.software.os.windows.WindowsNetworkParams.testGetNetworkParams(WindowsNetworkParams.java:229)
at oshi.software.os.windows.WindowsNetworkParams.main(WindowsNetworkParams.java:249)

Does not error out on my home machine (Mac running virtualized Windows 7) but I can reproduce at will on my work machine:

  • Microsoft Windows 10 Enterprise version 1607 build 14393.2248, 64-bit
  • Built using JavaSE-1.7 library from Oracle jdk1.8.0_172

Other notes:

  • Works without the read() because JNA follows the linked list by itself, so the read is redundant.
  • While run in isolation it takes several iterations, it usually generates an error the first time if I've done a lot of other, unrelated things in my program first.
  • As noted in the PR, works if you manually follow the pointers.
  • I've tried to add output and follow what's happening and believe it's related to caching the structure, but I get lost when it begins dealing with native memory.

@dbwiddis
Copy link
Contributor Author

So, I'm looking at this code again, and it just hit me. The error is occurring when I initialize FIXED_INFO with a brand new(?) Memory object.

So the initialization doesn't encounter problems when the new buffer is zeroed out (a "fresh" JVM) which has expected null values for the pointers, or if Java happens to recycle the same Memory object that it just released to the heap on the earlier iteration (which I now realize I was seeing in my debugging). But somehow it is occasionally claiming "used" memory that, on some occasions (apparently often on my work machine's JVM), produces a non-null pointer.

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Jul 19, 2018

And... suspicion confirmed. The error goes away when I replace

FIXED_INFO buffer = new FIXED_INFO(new Memory(bufferSize.getValue()));

with

Memory mem = new Memory(bufferSize.getValue());
mem.clear(); // <--- this line fixes the error
FIXED_INFO buffer = new FIXED_INFO(mem);

I can also "fix" the error by both forcing garbage collection and waiting at least 500 milliseconds before instantiating the new Memory object.

So I'm not sure whether it's Java side or C side or both, or whether it's my bug or JNA's, but at least I know what's happening now!

@twall
Copy link
Contributor

twall commented Jul 19, 2018 via email

@dbwiddis
Copy link
Contributor Author

Thanks, @twall. I'd come to that conclusion, that using the Pointer constructor as I did in the test case is the wrong approach. But given the linked list internally, there's no way for the code to know how big it needs to be so this will always need to have at least the size argument passed to the constructor. I suppose I can disable the no-arg constructor and require a size argument? Is there a standard pattern for this?

@dbwiddis
Copy link
Contributor Author

OK, I've got it all sorted out. The error was trying to cast the memory to FIXED_INFO before passing it to the GetNetworkParams function. I changed that function to accept a Pointer instead, and pass the allocated memory, which gets populated, and then the FIXED_INFO(Pointer) constructor just works like it is designed to.

Also the version of IPHlpAPI.java that I committed earlier had Windows line breaks. Putting them back to Unix.

@matthiasblaesing
Copy link
Member

Thank you for the fix - looks right. The explanation (in review with you observations) is in the definition of malloc (see C+ definition).It allocates memory, but does not initialize it. Running Memory#clear zeros the memory.

The change call sequence (allocate memory, let it be initialized by GetNetworkParams and then wrap it as a structure) prevents JNA trying to use uninitialized memory as a structure. Which happens, when you pass a freshly allocated Memory object to the structure constructor.

This is also explains why it works at first, as at that point you will get newly allocated memory, that is initialized by the OS, later you will get memory that the application already used and freed.

@matthiasblaesing matthiasblaesing merged commit 30cb928 into java-native-access:master Jul 20, 2018
@dbwiddis dbwiddis deleted the iphlp-bugfix branch July 21, 2018 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants