[plcrashreporter] push by landon.j.fuller@gmail.com - Fix incorrect encoding of the pid/ppid.... on 2013-07-10 16:32 GMT

[codesite...@google.com]

Revision: aec8b71dfe54
Author: Landon Fuller <lan...@plausible.coop>
Date: Wed Jul 10 09:29:28 2013
Log: Fix incorrect encoding of the pid/ppid.

Switches the encoding to the proto-defined uint32 type. As noted in the
commit comments, for correctness, this should be switched to int64 when
it's possible to deploy a breaking change to the protobuf serialization.

This commit also adds missing tests for the ProcessInfo fields (including
the pid fields), and enables AppInfo validation tests.

Issue: 66

http://code.google.com/p/plcrashreporter/source/detail?r=aec8b71dfe54

Modified:
/Source/PLCrashLogWriter.m
/Source/PLCrashLogWriterTests.m

=======================================
--- /Source/PLCrashLogWriter.m Fri Jan 4 18:53:26 2013
+++ /Source/PLCrashLogWriter.m Wed Jul 10 09:29:28 2013
@@ -645,12 +645,24 @@
{
size_t rv = 0;

+ /*
+ * In the current crash reporter serialization format, pid values are
serialized as unsigned 32-bit integers. This
+ * conforms with the actual implementation of pid_t on both 32-bit and
64-bit Darwin systems. To conform with
+ * SuSV3, however, the values should be encoded as signed integers;
the actual width of the type being implementation
+ * defined.
+ *
+ * To maintain compatibility with existing report readers the values
remain encoded as unsigned 32-bit integers,
+ * but should be updated to int64 values in future major revision of
the data format.
+ */
+ uint32_t pidval;
+
/* Process name */
if (process_name != NULL)
rv += plcrash_writer_pack(file,
PLCRASH_PROTO_PROCESS_INFO_PROCESS_NAME_ID, PLPROTOBUF_C_TYPE_STRING,
process_name);

/* Process ID */
- rv += plcrash_writer_pack(file,
PLCRASH_PROTO_PROCESS_INFO_PROCESS_ID_ID, PLPROTOBUF_C_TYPE_UINT64,
&process_id);
+ pidval = process_id;
+ rv += plcrash_writer_pack(file,
PLCRASH_PROTO_PROCESS_INFO_PROCESS_ID_ID, PLPROTOBUF_C_TYPE_UINT32,
&pidval);

/* Process path */
if (process_path != NULL)
@@ -661,7 +673,8 @@
rv += plcrash_writer_pack(file,
PLCRASH_PROTO_PROCESS_INFO_PARENT_PROCESS_NAME_ID,
PLPROTOBUF_C_TYPE_STRING, parent_process_name);

/* Parent process ID */
- rv += plcrash_writer_pack(file,
PLCRASH_PROTO_PROCESS_INFO_PARENT_PROCESS_ID_ID, PLPROTOBUF_C_TYPE_UINT64,
&parent_process_id);
+ pidval = parent_process_id;
+ rv += plcrash_writer_pack(file,
PLCRASH_PROTO_PROCESS_INFO_PARENT_PROCESS_ID_ID, PLPROTOBUF_C_TYPE_UINT32,
&pidval);

/* Native process. */
rv += plcrash_writer_pack(file, PLCRASH_PROTO_PROCESS_INFO_NATIVE_ID,
PLPROTOBUF_C_TYPE_BOOL, &native);
=======================================
--- /Source/PLCrashLogWriterTests.m Fri Jan 4 18:53:26 2013
+++ /Source/PLCrashLogWriterTests.m Wed Jul 10 09:29:28 2013
@@ -43,6 +43,8 @@

#import "crash_report.pb-c.h"

+#import "PLCrashSysctl.h"
+
@interface PLCrashLogWriterTests : SenTestCase {
@private
/* Path to crash log */
@@ -107,6 +109,64 @@
STAssertTrue(strcmp(appInfo->version, "1.0") == 0, @"Incorrect app
version written");
}

+// check a crash report's process info
+- (void) checkProcessInfo: (Plcrash__CrashReport *) crashReport {
+ Plcrash__CrashReport__ProcessInfo *procInfo =
crashReport->process_info;
+
+ STAssertNotNULL(procInfo, @"No process info available");
+ // Nothing else to do?
+ if (procInfo == NULL)
+ return;
+
+ STAssertEquals((pid_t)procInfo->process_id, getpid(), @"Incorrect
process id written");
+ STAssertEquals((pid_t)procInfo->parent_process_id, getppid(),
@"Incorrect parent process id written");
+
+ int retval;
+ if (plcrash_sysctl_int("sysctl.proc_native", &retval)) {
+ if (retval == 0) {
+ STAssertTrue(procInfo->native, @"Our current process is marked
as non-native");
+ } else {
+ STAssertTrue(procInfo->native, @"Our current process is marked
as native");
+ }
+ } else {
+ /* If the sysctl is not available, the process can be assumed to
be native. */
+ STAssertTrue(procInfo->native, @"No proc_native sysctl specified;
native should be assumed");
+ }
+
+ /* Fetch and verify process data via sysctl */
+ struct kinfo_proc process_info;
+ size_t process_info_len = sizeof(process_info);
+ int process_info_mib[4] = { CTL_KERN, KERN_PROC, KERN_PROC_PID, 0 };
+ int process_info_mib_len = 4;
+
+ /* Current process name name */
+ process_info_mib[3] = getpid();
+ if (sysctl(process_info_mib, process_info_mib_len, &process_info,
&process_info_len, NULL, 0) == 0) {
+ STAssertEqualCStrings(procInfo->process_name,
process_info.kp_proc.p_comm, @"Incorrect process name");
+ } else {
+ STFail(@"Could not retreive process name: %s", strerror(errno));
+ }
+
+ /* Current process path */
+ char *process_path = NULL;
+ uint32_t process_path_len = 0;
+
+ _NSGetExecutablePath(NULL, &process_path_len);
+ if (process_path_len > 0) {
+ process_path = malloc(process_path_len);
+ _NSGetExecutablePath(process_path, &process_path_len);
+ STAssertEqualCStrings(procInfo->process_path, process_path,
@"Incorrect process name");
+ free(process_path);
+ }
+
+ /* Parent process */
+ process_info_mib[3] = getppid();
+ if (sysctl(process_info_mib, process_info_mib_len, &process_info,
&process_info_len, NULL, 0) == 0) {
+ STAssertEqualCStrings(procInfo->parent_process_name,
process_info.kp_proc.p_comm, @"Incorrect process name");
+ } else {
+ STFail(@"Could not retreive parent process name: %s",
strerror(errno));
+ }
+}

- (void) checkThreads: (Plcrash__CrashReport *) crashReport {
Plcrash__CrashReport__Thread **threads = crashReport->threads;
@@ -277,6 +337,8 @@

/* Test the report */
[self checkSystemInfo: crashReport];
+ [self checkAppInfo: crashReport];
+ [self checkProcessInfo: crashReport];
[self checkThreads: crashReport];
[self checkException: crashReport];