Skip to content

Commit

Permalink
Try to avoid a JNA invalid memory error by avoiding the Memory finalizer
Browse files Browse the repository at this point in the history
That finalizer is known to prematurely free memory, in at least one
case[1].  I noticed a non-reproducible "Invalid memory error" from a call
to winpty_open that I speculate is a similar problem.

[1] java-native-access/jna#664
  • Loading branch information
rprichard committed May 31, 2016
1 parent d757ecd commit bbac3d4
Show file tree
Hide file tree
Showing 2 changed files with 174 additions and 28 deletions.
98 changes: 98 additions & 0 deletions src/com/pty4j/windows/ManualAllocation.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package com.pty4j.windows;

import com.sun.jna.Memory;
import com.sun.jna.Pointer;
import com.sun.jna.platform.win32.WinNT;

class ManualAllocation {

// Expose the Memory class' protected malloc/free functions. The JNA Memory
// class' finalizer is capable of freeing memory prematurely[1]. I saw the
// winpty_open call produce an "Invalid memory access" error (only once,
// non-reproducibly) when it used a PointerByReference object for its error
// output parameter. Perhaps the PointerByReference object was somehow
// finalized too soon? I'm skeptical, but avoiding the Memory finalizer
// seems prudent.
//
// The Manual{Int,HANDLE}ByReferences classes in this module must be
// explicitly closed. The Java 7 try-with-resources pattern works.
//
// [1] https://github.com/java-native-access/jna/issues/664

static class ManualMemory extends Memory {
static Pointer alloc(long size) {
long rawPtr = Memory.malloc(size);
if (rawPtr == 0) {
throw new OutOfMemoryError("Cannot allocate " + size + " bytes");
}
try {
Pointer ret = new Pointer(rawPtr);
rawPtr = 0;
return ret;
} finally {
if (rawPtr != 0) {
Memory.free(rawPtr);
}
}
}

static void free(Pointer ptr) {
Memory.free(Pointer.nativeValue(ptr));
}
}

static class ManualByReference implements AutoCloseable {
private Pointer ptr;

ManualByReference() {
ptr = ManualMemory.alloc(Pointer.SIZE);
}

Pointer byRef() {
return ptr;
}

@Override
public void close() {
ManualMemory.free(ptr);
ptr = null;
}
}

static class ManualHANDLEByReference extends ManualByReference {
ManualHANDLEByReference() {
this(null);
}

ManualHANDLEByReference(WinNT.HANDLE value) {
setValue(value);
}

void setValue(WinNT.HANDLE value) {
byRef().setPointer(0, value == null ? null : value.getPointer());
}

WinNT.HANDLE getValue() {
Pointer value = byRef().getPointer(0);
return value == null ? null : new WinNT.HANDLE(value);
}
}

static class ManualIntByReference extends ManualByReference {
ManualIntByReference() {
this(0);
}

ManualIntByReference(int value) {
setValue(value);
}

void setValue(int value) {
byRef().setInt(0, value);
}

int getValue() {
return byRef().getInt(0);
}
}
}
104 changes: 76 additions & 28 deletions src/com/pty4j/windows/WinPty.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import static com.sun.jna.platform.win32.WinBase.INFINITE;
import static com.sun.jna.platform.win32.WinNT.GENERIC_READ;
import static com.sun.jna.platform.win32.WinNT.GENERIC_WRITE;
import static com.pty4j.windows.ManualAllocation.ManualIntByReference;
import static com.pty4j.windows.ManualAllocation.ManualHANDLEByReference;

/**
* @author traff
Expand All @@ -31,20 +33,62 @@ public class WinPty {
private int myStatus = -1;
private boolean myClosed = false;

private static class WinptyError implements AutoCloseable {
private Pointer myErrPtr;

WinptyError() {
myErrPtr = ManualAllocation.ManualMemory.alloc(Pointer.SIZE);
myErrPtr.setPointer(0, null);
}

Pointer errorObject() {
return myErrPtr.getPointer(0);
}

void freeErrorObject() {
Pointer errorObject = errorObject();
if (errorObject != null) {
INSTANCE.winpty_error_free(errorObject);
myErrPtr.setPointer(0, null);
}
}

Pointer byRef() {
freeErrorObject();
return myErrPtr;
}

int code() {
return INSTANCE.winpty_error_code(errorObject());
}

String message() {
return INSTANCE.winpty_error_msg(errorObject()).toString();
}

@Override
public void close() {
freeErrorObject();
ManualAllocation.ManualMemory.free(myErrPtr);
myErrPtr = null;
}
}

public WinPty(String cmdline, String cwd, String env, boolean consoleMode) throws PtyException, IOException {
int cols = Integer.getInteger("win.pty.cols", 80);
int rows = Integer.getInteger("win.pty.rows", 25);

PointerByReference errPtr = new PointerByReference(null);
Pointer agentCfg = null;
Pointer spawnCfg = null;
Pointer winpty = null;
WinNT.HANDLEByReference processHandle = new WinNT.HANDLEByReference();
ManualHANDLEByReference processHandle = null;
NamedPipe coninPipe = null;
NamedPipe conoutPipe = null;
NamedPipe conerrPipe = null;

try {
try (WinptyError errPtr = new WinptyError()) {
processHandle = new ManualHANDLEByReference();

// Configure the winpty agent.
long agentFlags = 0;
if (consoleMode) {
Expand All @@ -57,10 +101,9 @@ public WinPty(String cmdline, String cwd, String env, boolean consoleMode) throw
INSTANCE.winpty_config_set_initial_size(agentCfg, cols, rows);

// Start the agent.
winpty = INSTANCE.winpty_open(agentCfg, errPtr);
winpty = INSTANCE.winpty_open(agentCfg, errPtr.byRef());
if (winpty == null) {
WString errMsg = INSTANCE.winpty_error_msg(errPtr.getValue());
throw new PtyException("Error starting winpty: " + errMsg.toString());
throw new PtyException("Error starting winpty: " + errPtr.message());
}

// Spawn a child process.
Expand All @@ -74,9 +117,8 @@ public WinPty(String cmdline, String cwd, String env, boolean consoleMode) throw
if (spawnCfg == null) {
throw new PtyException("winpty spawn cfg is null");
}
if (!INSTANCE.winpty_spawn(winpty, spawnCfg, processHandle, null, null, errPtr)) {
WString errMsg = INSTANCE.winpty_error_msg(errPtr.getValue());
throw new PtyException("Error running process: " + errMsg.toString());
if (!INSTANCE.winpty_spawn(winpty, spawnCfg, processHandle.byRef(), null, null, errPtr.byRef())) {
throw new PtyException("Error running process: " + errPtr.message());
}

// Connect the pipes. These calls return immediately (i.e. they don't block).
Expand All @@ -102,12 +144,16 @@ public WinPty(String cmdline, String cwd, String env, boolean consoleMode) throw
coninPipe = conoutPipe = conerrPipe = null;

} finally {
INSTANCE.winpty_error_free(errPtr.getValue());
// None of the code in this finally block should ever throw an exception.
INSTANCE.winpty_config_free(agentCfg);
INSTANCE.winpty_spawn_config_free(spawnCfg);
INSTANCE.winpty_free(winpty);
if (processHandle.getValue() != null) {
Kernel32.INSTANCE.CloseHandle(processHandle.getValue());
if (processHandle != null) {
WinNT.HANDLE handle = processHandle.getValue();
if (handle != null) {
Kernel32.INSTANCE.CloseHandle(handle);
}
processHandle.close();
}
closeNamedPipeQuietly(coninPipe);
closeNamedPipeQuietly(conoutPipe);
Expand Down Expand Up @@ -226,17 +272,17 @@ public NamedPipe getErrorPipe() {
// could be done using an extra reference count, or by using DuplicateHandle
// for INFINITE waits.
private class WaitForExitThread extends Thread {
private IntByReference myStatusByRef = new IntByReference(-1);

@Override
public void run() {
Kernel32.INSTANCE.WaitForSingleObject(myProcess, INFINITE);
Kernel32.INSTANCE.GetExitCodeProcess(myProcess, myStatusByRef);
synchronized (WinPty.this) {
WinPty.this.myChildExited = true;
WinPty.this.myStatus = myStatusByRef.getValue();
closeUnusedProcessHandle();
WinPty.this.notifyAll();
try (ManualIntByReference status = new ManualIntByReference(-1)) {
Kernel32.INSTANCE.WaitForSingleObject(myProcess, INFINITE);
KERNEL32.GetExitCodeProcess(myProcess, status.byRef());
synchronized (WinPty.this) {
WinPty.this.myChildExited = true;
WinPty.this.myStatus = status.getValue();
closeUnusedProcessHandle();
WinPty.this.notifyAll();
}
}
}
}
Expand Down Expand Up @@ -279,6 +325,8 @@ WinNT.HANDLE CreateNamedPipeA(String lpName,
boolean CancelIo(WinNT.HANDLE hFile);

int GetCurrentProcessId();

boolean GetExitCodeProcess(WinNT.HANDLE hProcess, Pointer lpExitCode);
}

public static WinPtyLib INSTANCE = (WinPtyLib)Native.loadLibrary(getLibraryPath(), WinPtyLib.class);
Expand Down Expand Up @@ -307,10 +355,10 @@ interface WinPtyLib extends Library {
WString winpty_error_msg(Pointer err);
void winpty_error_free(Pointer err);

Pointer winpty_config_new(long flags, PointerByReference err);
Pointer winpty_config_new(long flags, Pointer err);
void winpty_config_free(Pointer cfg);
void winpty_config_set_initial_size(Pointer cfg, int cols, int rows);
Pointer winpty_open(Pointer cfg, PointerByReference err);
Pointer winpty_open(Pointer cfg, Pointer err);

WString winpty_conin_name(Pointer wp);
WString winpty_conout_name(Pointer wp);
Expand All @@ -321,16 +369,16 @@ Pointer winpty_spawn_config_new(long flags,
WString cmdline,
WString cwd,
WString env,
PointerByReference err);
Pointer err);

void winpty_spawn_config_free(Pointer cfg);

boolean winpty_spawn(Pointer winpty,
Pointer cfg,
WinNT.HANDLEByReference process_handle,
WinNT.HANDLEByReference thread_handle,
IntByReference create_process_error,
PointerByReference err);
Pointer process_handle,
Pointer thread_handle,
Pointer create_process_error,
Pointer err);

boolean winpty_set_size(Pointer winpty, int cols, int rows, PointerByReference err);
void winpty_free(Pointer winpty);
Expand Down

0 comments on commit bbac3d4

Please sign in to comment.