Windows, JNI: use RAII pattern + clean up gotos
Introduce helper classes whose d'tor automatically
release resources (close handles, release Java
object handles, etc) and get rid of the
"goto cleanup" pattern.
--
PiperOrigin-RevId: 144349886
MOS_MIGRATED_REVID=144349886
diff --git a/src/main/native/windows_processes.cc b/src/main/native/windows_processes.cc
index 951ad77..0eb7e5f 100644
--- a/src/main/native/windows_processes.cc
+++ b/src/main/native/windows_processes.cc
@@ -76,6 +76,44 @@
error_("") {}
};
+struct AutoHandle {
+ AutoHandle() : handle(INVALID_HANDLE_VALUE) {}
+ ~AutoHandle() {
+ CloseHandle(handle); // handles INVALID_HANDLE_VALUE
+ handle = INVALID_HANDLE_VALUE;
+ }
+
+ HANDLE handle;
+};
+
+class JavaByteArray {
+ public:
+ JavaByteArray(JNIEnv* env, jbyteArray java_array)
+ : env_(env),
+ array_(java_array),
+ size_(java_array != nullptr ? env->GetArrayLength(java_array) : 0),
+ ptr_(java_array != nullptr ? env->GetByteArrayElements(java_array, NULL)
+ : nullptr) {}
+
+ ~JavaByteArray() {
+ if (array_ != nullptr) {
+ env_->ReleaseByteArrayElements(array_, ptr_, 0);
+ array_ = nullptr;
+ size_ = 0;
+ ptr_ = nullptr;
+ }
+ }
+
+ jsize size() { return size_; }
+ jbyte* ptr() { return ptr_; }
+
+ private:
+ JNIEnv* env_;
+ jbyteArray array_;
+ jsize size_;
+ jbyte* ptr_;
+};
+
static bool NestedJobsSupported() {
OSVERSIONINFOEX version_info;
version_info.dwOSVersionInfoSize = sizeof(version_info);
@@ -113,6 +151,8 @@
return (path.size() > MAX_PATH) ? (std::wstring(L"\\\\?\\") + path) : path;
}
+static jlong PtrAsJlong(void* p) { return reinterpret_cast<jlong>(p); }
+
extern "C" JNIEXPORT jlong JNICALL
Java_com_google_devtools_build_lib_windows_WindowsProcesses_nativeCreateProcess(
JNIEnv* env, jclass clazz, jstring java_argv0, jstring java_argv_rest,
@@ -127,9 +167,6 @@
AddUncPrefixMaybe(GetJavaWstring(env, java_stderr_redirect));
std::string cwd = GetJavaUTFString(env, java_cwd);
- jsize env_size = -1;
- jbyte* env_bytes = NULL;
-
std::unique_ptr<char[]> mutable_commandline(new char[commandline.size() + 1]);
strncpy(mutable_commandline.get(), commandline.c_str(),
commandline.size() + 1);
@@ -140,37 +177,39 @@
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
sa.bInheritHandle = TRUE;
- HANDLE stdin_process = INVALID_HANDLE_VALUE;
- HANDLE stdout_process = INVALID_HANDLE_VALUE;
- HANDLE stderr_process = INVALID_HANDLE_VALUE;
- HANDLE thread = INVALID_HANDLE_VALUE;
- HANDLE event = INVALID_HANDLE_VALUE;
+ // Standard file handles are closed even if the process was successfully
+ // created. If this was not so, operations on these file handles would not
+ // return immediately if the process is terminated.
+ // Therefore we make these handles auto-closing (by using AutoHandle).
+ AutoHandle stdin_process;
+ AutoHandle stdout_process;
+ AutoHandle stderr_process;
+ AutoHandle thread;
PROCESS_INFORMATION process_info = {0};
STARTUPINFOA startup_info = {0};
JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_info = {0};
- if (java_env != NULL) {
- env_size = env->GetArrayLength(java_env);
- env_bytes = env->GetByteArrayElements(java_env, NULL);
-
- if (env_size < 2) {
+ JavaByteArray env_map(env, java_env);
+ if (env_map.ptr() != nullptr) {
+ if (env_map.size() < 2) {
result->error_ = "The environment must be at least two bytes long";
- goto cleanup;
- } else if (env_bytes[env_size - 1] != 0 || env_bytes[env_size - 2] != 0) {
+ return PtrAsJlong(result);
+ } else if (env_map.ptr()[env_map.size() - 1] != 0 ||
+ env_map.ptr()[env_map.size() - 2] != 0) {
result->error_ = "Environment array must end with two null bytes";
- goto cleanup;
+ return PtrAsJlong(result);
}
}
- if (!CreatePipe(&stdin_process, &result->stdin_, &sa, 0)) {
+ if (!CreatePipe(&stdin_process.handle, &result->stdin_, &sa, 0)) {
result->error_ = windows_util::GetLastErrorString("CreatePipe(stdin)");
- goto cleanup;
+ return PtrAsJlong(result);
}
if (!stdout_redirect.empty()) {
result->stdout_.close();
- stdout_process = CreateFileW(
+ stdout_process.handle = CreateFileW(
/* lpFileName */ stdout_redirect.c_str(),
/* dwDesiredAccess */ FILE_APPEND_DATA,
/* dwShareMode */ 0,
@@ -179,23 +218,37 @@
/* dwFlagsAndAttributes */ FILE_ATTRIBUTE_NORMAL,
/* hTemplateFile */ NULL);
- if (stdout_process == INVALID_HANDLE_VALUE) {
+ if (stdout_process.handle == INVALID_HANDLE_VALUE) {
result->error_ = windows_util::GetLastErrorString("CreateFile(stdout)");
- goto cleanup;
+ return PtrAsJlong(result);
}
} else {
- if (!CreatePipe(&result->stdout_.handle_, &stdout_process, &sa, 0)) {
+ if (!CreatePipe(&result->stdout_.handle_, &stdout_process.handle, &sa, 0)) {
result->error_ = windows_util::GetLastErrorString("CreatePipe(stdout)");
- goto cleanup;
+ return PtrAsJlong(result);
}
}
+ // The value of the stderr HANDLE.
+ // If stdout and stderr are redirected to the same file, this will be equal to
+ // stdout_process.handle and stderr_process.handle will be
+ // INVALID_HANDLE_VALUE, so the two AutoHandle objects' d'tors will not
+ // attempt to close stdout's handle twice.
+ // If stdout != stderr, then stderr_handle = stderr_process.handle, and these
+ // are distinct from stdout_process.handle, so again the d'tors will do the
+ // right thing, closing the handles.
+ // In both cases, we DO NOT close stderr_handle, since it's either
+ // stdout_process's or stderr_process's d'tor doing so.
+ HANDLE stderr_handle = INVALID_HANDLE_VALUE;
+
if (!stderr_redirect.empty()) {
result->stderr_.close();
if (stdout_redirect == stderr_redirect) {
- stderr_process = stdout_process;
+ stderr_handle = stdout_process.handle;
+ // do not set stderr_process.handle; it equals stdout_process.handle and
+ // the AutoHandle d'tor would attempt to close it again
} else {
- stderr_process = CreateFileW(
+ stderr_handle = CreateFileW(
/* lpFileName */ stderr_redirect.c_str(),
/* dwDesiredAccess */ FILE_APPEND_DATA,
/* dwShareMode */ 0,
@@ -204,16 +257,20 @@
/* dwFlagsAndAttributes */ FILE_ATTRIBUTE_NORMAL,
/* hTemplateFile */ NULL);
- if (stderr_process == INVALID_HANDLE_VALUE) {
+ if (stderr_handle == INVALID_HANDLE_VALUE) {
result->error_ = windows_util::GetLastErrorString("CreateFile(stderr)");
- goto cleanup;
+ return PtrAsJlong(result);
}
+ // stderr_process != stdout_process, so set its handle, so the AutoHandle
+ // d'tor will close it
+ stderr_process.handle = stderr_handle;
}
} else {
- if (!CreatePipe(&result->stderr_.handle_, &stderr_process, &sa, 0)) {
+ if (!CreatePipe(&result->stderr_.handle_, &stderr_handle, &sa, 0)) {
result->error_ = windows_util::GetLastErrorString("CreatePipe(stderr)");
- goto cleanup;
+ return PtrAsJlong(result);
}
+ stderr_process.handle = stderr_handle;
}
// MDSN says that the default for job objects is that breakaway is not
@@ -221,7 +278,7 @@
HANDLE job = CreateJobObject(NULL, NULL);
if (job == NULL) {
result->error_ = windows_util::GetLastErrorString("CreateJobObject()");
- goto cleanup;
+ return PtrAsJlong(result);
}
result->job_ = job;
@@ -235,12 +292,12 @@
sizeof(job_info))) {
result->error_ =
windows_util::GetLastErrorString("SetInformationJobObject()");
- goto cleanup;
+ return PtrAsJlong(result);
}
- startup_info.hStdInput = stdin_process;
- startup_info.hStdOutput = stdout_process;
- startup_info.hStdError = stderr_process;
+ startup_info.hStdInput = stdin_process.handle;
+ startup_info.hStdOutput = stdout_process.handle;
+ startup_info.hStdError = stderr_handle;
startup_info.dwFlags |= STARTF_USESTDHANDLES;
BOOL ok = CreateProcessA(
@@ -252,19 +309,19 @@
/* dwCreationFlags */ CREATE_NO_WINDOW // Don't create a console window
| CREATE_NEW_PROCESS_GROUP // So that Ctrl-Break is not propagated
| CREATE_SUSPENDED, // So that it doesn't start a new job itself
- /* lpEnvironment */ env_bytes,
+ /* lpEnvironment */ env_map.ptr(),
/* lpCurrentDirectory */ cwd.empty() ? nullptr : cwd.c_str(),
/* lpStartupInfo */ &startup_info,
/* lpProcessInformation */ &process_info);
if (!ok) {
result->error_ = windows_util::GetLastErrorString("CreateProcess()");
- goto cleanup;
+ return PtrAsJlong(result);
}
result->pid_ = process_info.dwProcessId;
result->process_ = process_info.hProcess;
- thread = process_info.hThread;
+ thread.handle = process_info.hThread;
if (!AssignProcessToJobObject(result->job_, result->process_)) {
BOOL is_in_job = false;
@@ -280,44 +337,18 @@
} else {
result->error_ =
windows_util::GetLastErrorString("AssignProcessToJobObject()");
- goto cleanup;
+ return PtrAsJlong(result);
}
}
// Now that we put the process in a new job object, we can start executing it
- if (ResumeThread(thread) == -1) {
+ if (ResumeThread(thread.handle) == -1) {
result->error_ = windows_util::GetLastErrorString("ResumeThread()");
- goto cleanup;
+ return PtrAsJlong(result);
}
result->error_ = "";
-
-cleanup:
- // Standard file handles are closed even if the process was successfully
- // created. If this was not so, operations on these file handles would not
- // return immediately if the process is terminated.
- if (stdin_process != INVALID_HANDLE_VALUE) {
- CloseHandle(stdin_process);
- }
-
- if (stdout_process != INVALID_HANDLE_VALUE) {
- CloseHandle(stdout_process);
- }
-
- if (stderr_process != INVALID_HANDLE_VALUE
- && stderr_process != stdout_process) {
- CloseHandle(stderr_process);
- }
-
- if (thread != INVALID_HANDLE_VALUE) {
- CloseHandle(thread);
- }
-
- if (env_bytes != NULL) {
- env->ReleaseByteArrayElements(java_env, env_bytes, 0);
- }
-
- return reinterpret_cast<jlong>(result);
+ return PtrAsJlong(result);
}
extern "C" JNIEXPORT jint JNICALL
@@ -326,22 +357,20 @@
jint offset, jint length) {
NativeProcess* process = reinterpret_cast<NativeProcess*>(process_long);
- jsize array_size = env->GetArrayLength(java_bytes);
- if (offset < 0 || length <= 0 || offset > array_size - length) {
+ JavaByteArray bytes(env, java_bytes);
+ if (offset < 0 || length <= 0 || offset > bytes.size() - length) {
process->error_ = "Array index out of bounds";
return -1;
}
- jbyte* bytes = env->GetByteArrayElements(java_bytes, NULL);
DWORD bytes_written;
- if (!::WriteFile(process->stdin_, bytes + offset, length, &bytes_written,
- NULL)) {
+ if (!::WriteFile(process->stdin_, bytes.ptr() + offset, length,
+ &bytes_written, NULL)) {
process->error_ = windows_util::GetLastErrorString("WriteFile()");
bytes_written = -1;
}
- env->ReleaseByteArrayElements(java_bytes, bytes, 0);
process->error_ = "";
return bytes_written;
}
@@ -350,14 +379,14 @@
Java_com_google_devtools_build_lib_windows_WindowsProcesses_nativeGetStdout(
JNIEnv* env, jclass clazz, jlong process_long) {
NativeProcess* process = reinterpret_cast<NativeProcess*>(process_long);
- return reinterpret_cast<jlong>(&process->stdout_);
+ return PtrAsJlong(&process->stdout_);
}
extern "C" JNIEXPORT jlong JNICALL
Java_com_google_devtools_build_lib_windows_WindowsProcesses_nativeGetStderr(
JNIEnv* env, jclass clazz, jlong process_long) {
NativeProcess* process = reinterpret_cast<NativeProcess*>(process_long);
- return reinterpret_cast<jlong>(&process->stderr_);
+ return PtrAsJlong(&process->stderr_);
}
extern "C" JNIEXPORT jint JNICALL
@@ -367,8 +396,8 @@
NativeOutputStream* stream =
reinterpret_cast<NativeOutputStream*>(stream_long);
- jsize array_size = env->GetArrayLength(java_bytes);
- if (offset < 0 || length <= 0 || offset > array_size - length) {
+ JavaByteArray bytes(env, java_bytes);
+ if (offset < 0 || length <= 0 || offset > bytes.size() - length) {
stream->error_ = "Array index out of bounds";
return -1;
}
@@ -378,9 +407,9 @@
return 0;
}
- jbyte* bytes = env->GetByteArrayElements(java_bytes, NULL);
DWORD bytes_read;
- if (!::ReadFile(stream->handle_, bytes + offset, length, &bytes_read, NULL)) {
+ if (!::ReadFile(stream->handle_, bytes.ptr() + offset, length, &bytes_read,
+ NULL)) {
// Check if either the other end closed the pipe or we did it with
// NativeOutputStream.close() . In the latter case, we'll get a "system
// call interrupted" error.
@@ -396,7 +425,6 @@
stream->error_ = "";
}
- env->ReleaseByteArrayElements(java_bytes, bytes, 0);
return bytes_read;
}
