Add MacOS C Reference Implementation #2

mlmitch · 2024-12-06T14:12:35Z

Basic methods for efficient calculation exposed in header
Use an allocatable data structure as handle so data and resources can be reused
Unit testing implemented with CUnit

jonahtomrobinson · 2025-01-02T23:46:16Z

include/cpid/cpid_macos.h

+// Microsoft SAL annotations are not available on macOS
+// Define them to enable use for documentation purposes
+#ifndef _In_
+#define _In_
+#endif
+#ifndef _Out_
+#define _Out_
+#endif
+#ifndef _Inout_
+#define _Inout_
+#endif


You look to be using doxygen documentation comments for the API in this file. Would use of https://www.doxygen.nl/manual/commands.html#cmdparam be a better approach then these Windows-specific defines? They aren't the most OS agnostic.

mmm ... coming back to this, its a little bit silly to have these for non-windows since each header file is platform specific

jonahtomrobinson · 2025-01-02T23:50:33Z

include/cpid/cpid_macos.h

+ *
+ * @return 0 on success, -1 on error.
+ */
+int cpid_make_uuid(_In_ cpid_handle_t library_handle, _In_ uint32_t pid, _In_ uint64_t creation_time_unix_epoch_seconds, _In_ uint32_t creation_time_micros_offset, _Out_ uuid_t uuid);


Would pid_t be a more appropriate type for pid? On my machine (and most I believe) pid_t is signed, which makes me think an unsigned type may not be appropriate.

https://www.gnu.org/software/libc/manual/html_node/Process-Identification.html

Done.

I take your point about pid being signed. It might be a good idea to change the CPID spec so the pid part of digest inputs is an int64_t instead of uint64_t. This would technically be a breaking change, but no one is using the max range of uint64_t so I think it would be a good change in practice.

Thoughts?

jonahtomrobinson · 2025-01-02T23:52:44Z

src/constants.h

+#pragma once
+
+#define SHA256_BUFFER_SIZE 32
+#define MAX_PID 0x7FFFFFFF


This value is configurable and system dependent, does it make sense to define it ourselves?

On a standard Linux (potentially unix) system the max default is 32768.

Why are we defining this number in hex?

With making the PID field pid_t/int64_t, I've removed this check and the OS call will just fail.

jonahtomrobinson · 2025-01-02T23:54:19Z

src/constants.h

+#define RETURN_SUCCESS 0
+#define RETURN_ERROR -1


Personally this seems a tad overkill, these are very standard return values with standard meanings.
We've also specified the meaning of the return values explicitly in the API comments

@return 0 on success, -1 on error.

My biggest concern about this implementation is that every function returns only success or failure. If it's a failure, the caller has no way to determine why it has failed. Given that this is a "UNIXy" implementation, I'd have thought that either:

Each function would directly return 0 on success or else an error code from errno.h.

Each function would return success/failure and set errno to an error code from errno.h.

By way of comparison, each function in the Windows implementation returns a standard winerror.h code.

Another option would be just return true/false from stdbool and simplify the API if we simply don't care about HOW it failed.

got rid of constants. returning 0 and -1 directly

jonahtomrobinson · 2025-01-02T23:54:42Z

src/linux/CMakeLists.txt

We should remove this empty file.

empty windows and linuc cmake linux files are just there to establish directory structure

jonahtomrobinson · 2025-01-02T23:55:49Z

src/macos/CMakeLists.txt

+find_package(OpenSSL REQUIRED)
+
+find_library(IOKIT_LIBRARY IOKit)
+if(NOT IOKIT_LIBRARY)
+  message(FATAL_ERROR "IO Kit not found")
+endif()
+
+find_library(COREFOUNDATION_LIBRARY CoreFoundation)
+if(NOT COREFOUNDATION_LIBRARY)
+  message(FATAL_ERROR "Core Foundation not found")
+endif()


Should the README explain how to acquire these dependencies if they are not available by default? Should we be specifying minimum versions?

I believe these are part of the base macos installation

jonahtomrobinson · 2025-01-02T23:58:50Z

src/macos/cpid_macos.c

+#include <openssl/evp.h>
+
+#include "cpid/cpid_macos.h"
+#include "../constants.h"


IMO it is preferable to add directory one level above to the include search paths or explicitly include the path to files based on a single top level include. Relative search paths like these are prone to breaking and make it harder to integrate a library in my experience.

Got rid of constants.h so this no longer applies

jonahtomrobinson · 2025-01-02T23:59:28Z

src/macos/cpid_macos.c

+    char serial_number[MACOS_SERIAL_NUMBER_BUFFER_SIZE]; // 16 bytes
+    uuid_t hardware_uuid; // 16 bytes


Consider removing these comments, I don't think they add anything.

I actually think they have a usefulness. The MACOS_EXPECTED_DIGEST_INPUT_CONTENT_SIZE macro is mysteriously set to 88 and having field sizes in trailing comments like this helps explain where that value comes from. I'd actually be inclined to add a similar trailing comment to the other fields in this struct, e.g.

typedef struct { char serial_number[MACOS_SERIAL_NUMBER_BUFFER_SIZE]; // 16 bytes uuid_t hardware_uuid; // + 16 bytes process_creation_time_t kernel_task_creation_time; // + 16 bytes process_creation_time_t launchd_creation_time; // + 16 bytes process_creation_time_t process_creation_time; // + 16 bytes uint64_t pid; // + 8 bytes } digest_input_content_t; // = 88 bytes

You could go either way for sure. If we keep them then I agree that we should be consistent and add them for all. But equally I would question what value the reader gets from knowing this information and whether we are adding comment management overhead (although admittedly minimal) to changes to this struct.

Added comments like dave has. The byte arithmetic for the structure should not change as this is the thing that gets hashed.

jonahtomrobinson · 2025-01-03T00:03:48Z

src/macos/cpid_macos.c

+
+int cpid_get_uuid_string(_In_ cpid_handle_t library_handle, _In_ uint32_t pid, _Out_ uuid_string_t uuid_string) {
+
+    if (NULL == library_handle || NULL == uuid_string) {


Consider the following styling.

Suggested change

if (NULL == library_handle || NULL == uuid_string) {

if (!library_handle || !uuid_string) {

I agree. A NULL == foo or foo == NULL test always makes me feel like I'm reading Java or C# 😄

Note that this practice is throughout this code.

jonahtomrobinson · 2025-01-03T00:04:46Z

src/macos/cpid_macos.c

+        return RETURN_ERROR;
+    }
+
+    uuid_t uuid;


Most standard c-styling would suggest declaring this at the top of the scope. What styling are you following in this project? It should probably be specified in the README.

Dunno about that tbh...we're not writing K&R C here.

I'm not really a proponent for it, rather I'm just picking up on the fact now as it's something that often comes up once there are more eyes on the code. Simply linking to some public styling guide or tool in the README would put this kind of comment to rest from the start.

What styling are you following in this project?

declare a variable as close as possible to its use

jonahtomrobinson · 2025-01-03T00:07:28Z

src/macos/cpid_macos.c

+    // set the uuid version (UUIDv8)
+    library_handle->digest_destination_buffer[6] = (library_handle->digest_destination_buffer[6] & 0x0F) | 0x80;
+
+    // set the uuid variant
+    library_handle->digest_destination_buffer[8] = (library_handle->digest_destination_buffer[8] & 0x3F) | 0x80;


Consider defining these magic numbers as constants and remove the comments. That why the code is (more) self-documenting.

jonahtomrobinson · 2025-01-03T00:09:33Z

src/macos/cpid_macos.c

+    if (NULL == library_handle || pid > MAX_PID || creation_time_micros_offset > MAX_MICROS_OFFSET || NULL == uuid) {
+        return RETURN_ERROR;
+    }


I'd consider breaking these checks up.

First validate the basic null check on pointer params, then perform further validation on the param data itself.

jonahtomrobinson · 2025-01-03T00:11:17Z

src/macos/cpid_macos.c

+        cpid_finalize(library_handle);
+        library_handle = NULL;


Would it be more flexible to move the NULL set into the cpid_finalize function?

But then you'd have to pass the address of that pointer, and that would potentially complicate calling this function from elsewhere. Consider, for example, that in other context where it is called the variable might be const.

It's not a big deal to me either way

jonahtomrobinson · 2025-01-03T00:15:54Z

src/macos/cpid_macos.c

+    int return_code = sysctl(mib, 4, &process_info, &info_size, NULL, 0);
+    if (0 != return_code) {
+        return RETURN_ERROR;
+    }


return_code doesn't seem to be used elsewhere and isn't used as a "return_code" in the context of this funciton. I'd suggest removing it.

Suggested change

int return_code = sysctl(mib, 4, &process_info, &info_size, NULL, 0);

if (0 != return_code) {

return RETURN_ERROR;

}

if (0 != sysctl(mib, 4, &process_info, &info_size, NULL, 0)) {

return RETURN_ERROR;

}

There is similar code in other functions in this file

The reason people traditionally do things like that is that it makes debugging easier because the value returned by the function call isn't lost. In a release build, the temporary variable will be optimised away anyway.

An obvious exception is where the return type of the called function is boolean. In that case, the value is clear from the direction that the if-statement takes 😄

Yeah I get it will be optimized away. If you're at the point of step-through debugging it is easy enough to break out the information you want if needed. To me defining a variable for it it to be basically unused seems verbose.

Certainly not a deal breaker either way.

jonahtomrobinson · 2025-01-03T00:19:24Z

test/windows/CMakeLists.txt

Empty file, should be removed

jonahtomrobinson · 2025-01-03T00:21:57Z

README.md

+Compile:
+```
+mkdir build && cd build
+cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON ..
+make
+ctest
+```


Defining a CMakePresets.txt could potentially simplify these steps.

davemcincork

One other major comment I have relates to const correctness. But let's discuss offline so that I'm clearer about the intention behind this code.

davemcincork · 2025-01-06T17:51:19Z

src/macos/cpid_macos.c

+    char serial_number[MACOS_SERIAL_NUMBER_BUFFER_SIZE]; // 16 bytes
+    uuid_t hardware_uuid; // 16 bytes


I actually think they have a usefulness. The MACOS_EXPECTED_DIGEST_INPUT_CONTENT_SIZE macro is mysteriously set to 88 and having field sizes in trailing comments like this helps explain where that value comes from. I'd actually be inclined to add a similar trailing comment to the other fields in this struct, e.g.

typedef struct { char serial_number[MACOS_SERIAL_NUMBER_BUFFER_SIZE]; // 16 bytes uuid_t hardware_uuid; // + 16 bytes process_creation_time_t kernel_task_creation_time; // + 16 bytes process_creation_time_t launchd_creation_time; // + 16 bytes process_creation_time_t process_creation_time; // + 16 bytes uint64_t pid; // + 8 bytes } digest_input_content_t; // = 88 bytes

davemcincork · 2025-01-06T17:55:34Z

src/macos/cpid_macos.c

+    int return_code = sysctl(mib, 4, &process_info, &info_size, NULL, 0);
+    if (0 != return_code) {
+        return RETURN_ERROR;
+    }


The reason people traditionally do things like that is that it makes debugging easier because the value returned by the function call isn't lost. In a release build, the temporary variable will be optimised away anyway.

An obvious exception is where the return type of the called function is boolean. In that case, the value is clear from the direction that the if-statement takes 😄

davemcincork · 2025-01-06T18:01:08Z

src/macos/cpid_macos.c

+        cpid_finalize(library_handle);
+        library_handle = NULL;


But then you'd have to pass the address of that pointer, and that would potentially complicate calling this function from elsewhere. Consider, for example, that in other context where it is called the variable might be const.

davemcincork · 2025-01-06T18:03:13Z

include/cpid/cpid_macos.h

+#define _Inout_
+#endif
+
+typedef struct cpid_struct *cpid_handle_t;


I think cpid_handle_t should be an opaque void* as it is in the Windows implementation.

davemcincork · 2025-01-06T18:06:39Z

src/macos/cpid_macos.c

+
+#pragma pack(push, 1)
+typedef struct {
+    uint64_t unix_epoch_seconds;


Don't you need to include <stdint.h> to get the fixed width integer types? If it's working now, it's most likely just a happy side-effect of including another header.

davemcincork · 2025-01-06T18:08:36Z

src/macos/cpid_macos.c

+    EVP_MD *sha256;
+    uint8_t digest_destination_buffer[EVP_MAX_MD_SIZE];
+    digest_input_content_t digest_input_content;
+} *cpid_handle_t;


As noted above, I don't think you should declaratively associate cpid_handle_t and cpid_struct.

davemcincork · 2025-01-06T18:24:39Z

src/macos/cpid_macos.c

+
+int cpid_get_uuid_string(_In_ cpid_handle_t library_handle, _In_ uint32_t pid, _Out_ uuid_string_t uuid_string) {
+
+    if (NULL == library_handle || NULL == uuid_string) {


I agree. A NULL == foo or foo == NULL test always makes me feel like I'm reading Java or C# 😄

Note that this practice is throughout this code.

davemcincork · 2025-01-06T18:26:10Z

src/macos/cpid_macos.c

+        return RETURN_ERROR;
+    }
+
+    uuid_t uuid;


Dunno about that tbh...we're not writing K&R C here.

davemcincork · 2025-01-06T18:38:47Z

src/constants.h

+#define RETURN_SUCCESS 0
+#define RETURN_ERROR -1


My biggest concern about this implementation is that every function returns only success or failure. If it's a failure, the caller has no way to determine why it has failed. Given that this is a "UNIXy" implementation, I'd have thought that either:

Each function would directly return 0 on success or else an error code from errno.h.

Each function would return success/failure and set errno to an error code from errno.h.

By way of comparison, each function in the Windows implementation returns a standard winerror.h code.

Update digest input to use signed integers Basic methods for efficient calculation exposed in header Use an allocatable data structure as handle so data and resources can be reused Unit testing implemented with CUnit Some const-correctness implemented Signed-off-by: Mitchell Wasson <[email protected]>

mikeradka requested a review from rmouritzen-splunk December 11, 2024 19:33

jonahtomrobinson reviewed Jan 2, 2025

View reviewed changes

jonahtomrobinson reviewed Jan 3, 2025

View reviewed changes

test/windows/CMakeLists.txt Outdated

Copy link

jonahtomrobinson Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty file, should be removed

jonahtomrobinson reviewed Jan 3, 2025

View reviewed changes

davemcincork reviewed Jan 6, 2025

View reviewed changes

mlmitch force-pushed the ref_impl branch from d9486de to 658bace Compare February 7, 2025 21:42

mlmitch force-pushed the ref_impl branch from 658bace to 07b9f0e Compare February 7, 2025 21:44

		char serial_number[MACOS_SERIAL_NUMBER_BUFFER_SIZE]; // 16 bytes
		uuid_t hardware_uuid; // 16 bytes


		int cpid_get_uuid_string(_In_ cpid_handle_t library_handle, _In_ uint32_t pid, _Out_ uuid_string_t uuid_string) {

		if (NULL == library_handle \|\| NULL == uuid_string) {

	if (NULL == library_handle \|\| NULL == uuid_string) {
	if (!library_handle \|\| !uuid_string) {

		#define RETURN_SUCCESS 0
		#define RETURN_ERROR -1

		#define RETURN_SUCCESS 0
		#define RETURN_ERROR -1

Add MacOS C Reference Implementation #2

Are you sure you want to change the base?

Add MacOS C Reference Implementation #2

Conversation

mlmitch commented Dec 6, 2024

Choose a reason for hiding this comment

mlmitch Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlmitch Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

jonahtomrobinson Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

mlmitch Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtomrobinson Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

mlmitch Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlmitch Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtomrobinson Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtomrobinson Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davemcincork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlmitch Feb 7, 2025 •

edited

Loading

mlmitch Feb 7, 2025 •

edited

Loading

jonahtomrobinson Jan 2, 2025 •

edited

Loading

mlmitch Feb 7, 2025 •

edited

Loading

jonahtomrobinson Jan 2, 2025 •

edited

Loading

mlmitch Feb 7, 2025 •

edited

Loading

mlmitch Feb 7, 2025 •

edited

Loading

jonahtomrobinson Jan 6, 2025 •

edited

Loading

jonahtomrobinson Jan 6, 2025 •

edited

Loading