Get real MacOS product version if in compability mode #6056

mikezhang1234567890 · 2021-06-08T23:52:26Z

If a program is built by a 10.15 or earlier SDK is run on Big Sur (version 11)
or newer, it will run in a backwards compatibility context which will return
product version as 10.16.

Thus, when we need to get Mac OS version when running on 11+, we need to use
a workaround to get the real version string. In this case, we look at a symlink
to the SystemVersion.plist file which bypasses the backwards compatibility
context.

Signed-off-by: Mike Zhang mike.h.zhang@ibm.com

Fixes eclipse-openj9/openj9#12630

0xdaryl · 2021-06-09T10:38:24Z

@tajila @pshipton : could one of you (or delegate) review these changes please?

pshipton · 2021-06-09T12:33:07Z

port/unix/omrsysinfo.c

+				* real version info.
+				*/
+				if (0 == strncmp(buffer, "10.16", 5)) {
+					portLibrary->mem_free_memory(portLibrary, buffer);


This should ~~first check buffer != myBuffer, or~~ free allocatedFileBuffer ~~instead using the same comment as below where allocatedFileBuffer is freed~~. Also set allocatedFileBuffer to NULL so it can't be freed twice. allocatedFileBuffer should be set to NULL above as well where it's freed.

Freed allocatedFileBuffer here.

buffer is NUL-terminated; shouldn't this use strcmp() instead?

pshipton · 2021-06-09T12:49:42Z

port/unix/omrsysinfo.c

+									len = resultSize + 1;
+									buffer = portLibrary->mem_allocate_memory(portLibrary, len,
+										OMR_GET_CALLSITE(), OMRMEM_CATEGORY_PORT_LIBRARY);
+									strncpy(buffer, versionString, len);


buffer needs to be checked for NULL

pshipton · 2021-06-09T12:52:53Z

port/unix/omrsysinfo.c

+						portLibrary->file_close(portLibrary, plistFile);
+						if (rc < 0) {
+							/* free the buffer if it was allocated */
+							portLibrary->mem_free_memory(portLibrary, allocatedFileBuffer);


This should set allocatedFileBuffer to NULL since it's freed again below.

mikezhang1234567890 · 2021-06-09T15:44:13Z

Can you please take another look @pshipton ?

pshipton · 2021-06-09T16:23:15Z

port/unix/omrsysinfo.c

+									if (NULL != buffer) {
+										strncpy(buffer, versionString, len);
+									} else {
+										rc = -1; /* error allocating memory */


I think the redundant code should be removed to make this easier to understand. This isn't needed.

pshipton · 2021-06-09T16:23:24Z

port/unix/omrsysinfo.c

+										rc = -1; /* error allocating memory */
+									}
+								} else {
+									rc = -1; /* parse error */


This isn't needed.

pshipton · 2021-06-09T16:23:59Z

port/unix/omrsysinfo.c

+							}
+						}
+						portLibrary->file_close(portLibrary, plistFile);
+						if (rc < 0) {


This block isn't needed, the memory is freed below regardless of the outcome.

pshipton · 2021-06-09T16:25:19Z

port/unix/omrsysinfo.c

+							portLibrary->mem_free_memory(portLibrary, allocatedFileBuffer);
+							allocatedFileBuffer = NULL;
+						}
+					} else { /* plistFile >= 0 */


This block isn't needed, rc isn't used for anything below.

pshipton · 2021-06-09T16:29:36Z

port/unix/omrsysinfo.c

+
+					intptr_t plistFile = portLibrary->file_open(portLibrary, COMPAT_PLIST_LINK, EsOpenRead, 0444);
+					if (plistFile >= 0) {
+						char myBuffer[1024];  /* the file size is ~500 bytes, 525 on a 11.3 */


Might as well remove this to save stack and use the previous definition of myBuffer, I think it's still in scope.

Doesn't look like it. The previous definition of myBuffer is bound to the previous read of the plist file that occurs if sysctl failed.

Ya ok, I see it now.

mikezhang1234567890 · 2021-06-09T20:30:04Z

Updated the PR again @pshipton

pshipton · 2021-06-09T21:27:24Z

port/unix/omrsysinfo.c

+								OMR_GET_CALLSITE(), OMRMEM_CATEGORY_PORT_LIBRARY);
+							fileBuffer = allocatedFileBuffer;
+						}
+						if (NULL != fileBuffer) {


There is a bug here (and the previous code). If file_length() fails then fileBuffer != NULL and len is negative and the fileBuffer[len] = '\0' below writes outside the bounds of the stack buffer. Also strstr() will be checking random data and may go outside the bounds of readable memory, or find a false hit (however unlikely) causing another write outside of the buffer.

Updated it so we look at the return value of file_flength() first before assigning fileBuffer. If the return is an error value, fileBuffer remains NULL.

Looks good, but how about assigning directly to fileLen, I don't why rc needs to be involved.

The return value of file_flength() is negative when an error occurs, and fileLen is an unsigned integer type.

size_t doesn't need to be used, file_flength() returns int64_t and the code could just keep using that. I'll defer to @keithc-ca

keithc-ca · 2021-06-10T16:01:00Z

port/unix/omrsysinfo.c

+				* real version info.
+				*/
+				if (0 == strncmp(buffer, "10.16", 5)) {
+					portLibrary->mem_free_memory(portLibrary, buffer);


buffer is NUL-terminated; shouldn't this use strcmp() instead?

keithc-ca · 2021-06-10T16:02:16Z

port/unix/omrsysinfo.c

+							while (bytesRemaining > 0) {
+								intptr_t result = portLibrary->file_read(portLibrary, plistFile, readPtr, bytesRemaining);
+								if (result > 0) {
+									bytesRemaining -= result;


Mixing signed and unsigned types here, suggest:

bytesRemaining -= (size_t) result;

keithc-ca · 2021-06-10T16:05:35Z

port/unix/omrsysinfo.c

+									buffer = portLibrary->mem_allocate_memory(portLibrary, len,
+										OMR_GET_CALLSITE(), OMRMEM_CATEGORY_PORT_LIBRARY);
+									if (NULL != buffer) {
+										strncpy(buffer, versionString, len);


Rather than write the NUL to readPtr above, I suggest this should use memcpy():

memcpy(buffer, versionString, resultSize); buffer[resultSize] = '\0';

mikezhang1234567890 · 2021-06-10T21:32:19Z

Updated PR again.

keithc-ca · 2021-06-11T19:24:49Z

port/unix/omrsysinfo.c

 				} else {
 					rc = sysctlbyname(KERN_OSPRODUCTVERSION, buffer, &resultSize, NULL, 0);
 				}
 				buffer[resultSize] = '\0';


buffer may refer to freed memory here.

There's no mem_free_memory called between when buffer was declared in L1999 and here. I don't see how.

Yes, you are correct. I read the code as if the reference was to the buffer declared on line 1940 (and possibly freed on line 1988).

pshipton · 2021-06-21T14:51:51Z

What's the next step for this, are all your review comments addressed @keithc-ca ?

keithc-ca · 2021-06-21T15:11:43Z

port/unix/omrsysinfo.c

+				* so we check the plink symlink as that bypasses the compatibility context and returns the
+				* real version info.
+				*/


nit: asterisks should line up

Updated PR to address this formatting issue.

keithc-ca · 2021-06-21T15:12:28Z

Other than the minor formatting nit, I think this looks good.

If a program is built by a 10.15 or earlier SDK is run on Big Sur (version 11) or newer, it will run in a backwards compatibility context which will return product version as 10.16. Thus, when we need to get Mac OS version when running on 11+, we need to use a workaround to get the real version string. In this case, we look at a symlink to the SystemVersion.plist file which bypasses the backwards compatibility context. Signed-off-by: Mike Zhang <mike.h.zhang@ibm.com>

pshipton · 2021-06-21T16:22:03Z

@0xdaryl I think this is ready to be tested.

0xdaryl · 2021-06-21T16:59:37Z

Jenkins build all

mikezhang1234567890 requested review from charliegracie, rwy7 and youngar as code owners June 8, 2021 23:52

github-actions bot added the comp:port label Jun 9, 2021

pshipton reviewed Jun 9, 2021

View reviewed changes

mikezhang1234567890 force-pushed the osx-version branch from 7f62913 to b31125e Compare June 9, 2021 15:41

pshipton reviewed Jun 9, 2021

View reviewed changes

mikezhang1234567890 force-pushed the osx-version branch from b31125e to b94d6e9 Compare June 9, 2021 17:05

pshipton reviewed Jun 9, 2021

View reviewed changes

mikezhang1234567890 force-pushed the osx-version branch from b94d6e9 to 16777b4 Compare June 10, 2021 00:18

keithc-ca reviewed Jun 10, 2021

View reviewed changes

mikezhang1234567890 force-pushed the osx-version branch from 16777b4 to 2551607 Compare June 10, 2021 21:19

keithc-ca suggested changes Jun 11, 2021

View reviewed changes

pshipton approved these changes Jun 21, 2021

View reviewed changes

keithc-ca reviewed Jun 21, 2021

View reviewed changes

mikezhang1234567890 force-pushed the osx-version branch from 2551607 to 87fba64 Compare June 21, 2021 16:04

keithc-ca approved these changes Jun 21, 2021

View reviewed changes

0xdaryl self-assigned this Jun 21, 2021

0xdaryl approved these changes Jun 21, 2021

View reviewed changes

0xdaryl merged commit 708c852 into eclipse-omr:master Jun 21, 2021

pshipton mentioned this pull request Jun 22, 2021

(0.27) Get real MacOS product version if in compability mode eclipse-openj9/openj9-omr#113

Merged

Get real MacOS product version if in compability mode #6056

Get real MacOS product version if in compability mode #6056

Uh oh!

Conversation

mikezhang1234567890 commented Jun 8, 2021

Uh oh!

0xdaryl commented Jun 9, 2021

Uh oh!

pshipton Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikezhang1234567890 commented Jun 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikezhang1234567890 Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikezhang1234567890 commented Jun 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshipton Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikezhang1234567890 commented Jun 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshipton commented Jun 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithc-ca commented Jun 21, 2021

Uh oh!

pshipton commented Jun 21, 2021

Uh oh!

0xdaryl commented Jun 21, 2021

Uh oh!

Reviewers

Assignees

pshipton Jun 9, 2021 •

edited

Loading

mikezhang1234567890 Jun 9, 2021 •

edited

Loading

pshipton Jun 10, 2021 •

edited

Loading