From c8376ec55b995efa02950ee81d4dd92e9b612328 Mon Sep 17 00:00:00 2001 From: Mike Zhang Date: Thu, 27 May 2021 13:24:43 -0400 Subject: [PATCH] Address issues raised in comments Fix formatting and style isues Change mach header to not dynamically allocate memory Handle pwrite errors. Signed-off-by: Mike Zhang --- port/osx/omrosdump.c | 209 ++++++++++++++++++++++++++----------------- 1 file changed, 129 insertions(+), 80 deletions(-) diff --git a/port/osx/omrosdump.c b/port/osx/omrosdump.c index a87acfbf722..c95aa7c63b1 100644 --- a/port/osx/omrosdump.c +++ b/port/osx/omrosdump.c @@ -59,13 +59,14 @@ struct thread_command_full_64 { static char corefile_name[PATH_MAX]; static int corefile_fd = -1; -static int coredump_to_file(mach_port_t, pid_t); -static int list_thread_commands(mach_port_t, struct thread_command_full_64 **, natural_t *); -static int list_segment_commands(mach_port_t, struct segment_command_64 **, natural_t *); +static kern_return_t coredump_to_file(mach_port_t, pid_t); +static kern_return_t list_thread_commands(mach_port_t, struct thread_command_full_64 **, natural_t *); +static kern_return_t list_segment_commands(mach_port_t, struct segment_command_64 **, natural_t *); -static int coredump_to_file(mach_port_t task_port, pid_t pid) { +static kern_return_t coredump_to_file(mach_port_t task_port, pid_t pid) +{ kern_return_t kr = KERN_SUCCESS; - struct mach_header_64 *mh64; + struct mach_header_64 mh64; struct segment_command_64 *segments = NULL; struct thread_command_full_64 *threads = NULL; natural_t segment_count; @@ -75,6 +76,7 @@ static int coredump_to_file(mach_port_t task_port, pid_t pid) { mach_msg_type_number_t info_count; off_t file_off = 0; uint64_t seg_file_off = 0; + ssize_t written = 0; corefile_fd = open(corefile_name, O_RDWR | O_CREAT | O_EXCL, 0600); @@ -83,90 +85,133 @@ static int coredump_to_file(mach_port_t task_port, pid_t pid) { goto done; } - kr = host_processor_info(mach_host_self(), PROCESSOR_BASIC_INFO, &cpu_count, (processor_info_array_t *)&proc_info_array, &info_count); - if (kr != KERN_SUCCESS) { + kr = host_processor_info(mach_host_self(), PROCESSOR_BASIC_INFO, &cpu_count, + (processor_info_array_t *)&proc_info_array, &info_count); + if (KERN_SUCCESS != kr) { mach_error("failed to get processor info:\n", kr); goto done; } - mh64 = calloc(1, sizeof(struct mach_header_64)); - mh64->magic = MH_MAGIC_64; - /* host_processor_info does not return a value with the 64-bit flag set so set manually since we only deal with 64-bit on OSX*/ - mh64->cputype = proc_info_array[0].cpu_type | CPU_ARCH_ABI64; - mh64->cpusubtype = proc_info_array[0].cpu_subtype; + mh64.magic = MH_MAGIC_64; + /* host_processor_info does not return a value with the 64-bit flag set so + * set manually since we only deal with 64-bit on OSX + */ + mh64.cputype = proc_info_array[0].cpu_type | CPU_ARCH_ABI64; + mh64.cpusubtype = proc_info_array[0].cpu_subtype; /* update ncmds and sizeofcmds after processing thread and segment commands */ - mh64->ncmds = 0; - mh64->sizeofcmds = 32; - mh64->filetype = MH_CORE; + mh64.ncmds = 0; + mh64.sizeofcmds = 32; + mh64.filetype = MH_CORE; file_off += sizeof(struct mach_header_64); segments = NULL; kr = list_thread_commands(task_port, &threads, &thread_count); - if (kr != KERN_SUCCESS) { + if (KERN_SUCCESS != kr) { mach_error("error getting thread command data:\n", kr); goto done; } - for (int i = 0; i < thread_count; i++) { + for (natural_t i = 0; i < thread_count; i++) { /* write each portion separately due to structure padding */ - pwrite(corefile_fd, &threads[i].cmd, 4, file_off); - pwrite(corefile_fd, &threads[i].cmdsize, 4, file_off + 4); + written = pwrite(corefile_fd, &threads[i].cmd, 4, file_off); + if (written < 0) { + perror("pwrite() error writing threads:"); + kr = KERN_FAILURE; + goto done; + } + written = pwrite(corefile_fd, &threads[i].cmdsize, 4, file_off + 4); + if (written < 0) { + perror("pwrite() error writing threads:"); + kr = KERN_FAILURE; + goto done; + } + file_off += 8; - pwrite(corefile_fd, &threads[i].thread_state, sizeof(x86_thread_state_t), file_off); + written = pwrite(corefile_fd, &threads[i].thread_state, sizeof(x86_thread_state_t), file_off); + if (written < 0) { + perror("pwrite() error writing threads:"); + kr = KERN_FAILURE; + goto done; + } file_off += sizeof(x86_thread_state_t); - pwrite(corefile_fd, &threads[i].float_state, sizeof(x86_float_state_t), file_off); + written = pwrite(corefile_fd, &threads[i].float_state, sizeof(x86_float_state_t), file_off); + if (written < 0) { + perror("pwrite() error writing threads:"); + kr = KERN_FAILURE; + goto done; + } file_off += sizeof(x86_float_state_t); - pwrite(corefile_fd, &threads[i].exceptions, sizeof(x86_exception_state_t), file_off); + written = pwrite(corefile_fd, &threads[i].exceptions, sizeof(x86_exception_state_t), file_off); + if (written < 0) { + perror("pwrite() error writing thread commands:"); + kr = KERN_FAILURE; + goto done; + } file_off += sizeof(x86_exception_state_t); - mh64->sizeofcmds += threads[i].cmdsize; + mh64.sizeofcmds += threads[i].cmdsize; } - mh64->ncmds += thread_count; + mh64.ncmds += thread_count; kr = list_segment_commands(task_port, &segments, &segment_count); - if (kr != KERN_SUCCESS) { - mach_error("error getting thread command data:\n", kr); + if (KERN_SUCCESS != kr) { + mach_error("error getting segment command data:\n", kr); goto done; } seg_file_off = file_off + segment_count * sizeof(struct segment_command_64); - for (int i = 0; i < segment_count; i++) { + for (natural_t i = 0; i < segment_count; i++) { vm_offset_t data_read; mach_msg_type_number_t data_size; segments[i].fileoff = seg_file_off; - pwrite(corefile_fd, &segments[i], sizeof(struct segment_command_64), file_off); + written = pwrite(corefile_fd, &segments[i], sizeof(struct segment_command_64), file_off); + if (written < 0) { + perror("pwrite() error writing segment commands:"); + kr = KERN_FAILURE; + goto done; + } kr = mach_vm_read(task_port, segments[i].vmaddr, segments[i].vmsize, &data_read, &data_size); - ssize_t written; written = pwrite(corefile_fd, (void *)data_read, data_size, seg_file_off); + if (written < 0) { + perror("pwrite() error writing segment data:"); + kr = KERN_FAILURE; + goto done; + } mach_vm_deallocate(task_port, data_read, data_size); file_off += sizeof(struct segment_command_64); seg_file_off += segments[i].vmsize; } - mh64->ncmds += segment_count; - mh64->sizeofcmds += segment_count * sizeof(struct segment_command_64); + mh64.ncmds += segment_count; + mh64.sizeofcmds += segment_count * sizeof(struct segment_command_64); /* write mach header after all command number and size are known */ - pwrite(corefile_fd, mh64, sizeof(struct mach_header_64), 0); + pwrite(corefile_fd, &mh64, sizeof(struct mach_header_64), 0); + if (written < 0) { + perror("pwrite() error writing mach header:"); + kr = KERN_FAILURE; + goto done; + } done: if (corefile_fd > 0) { close(corefile_fd); - if (kr != KERN_SUCCESS) { + if (KERN_SUCCESS != kr) { remove(corefile_name); } } return kr; } -static int list_thread_commands(mach_port_t task_port, struct thread_command_full_64 **thread_commands, natural_t *thread_count) { +static kern_return_t list_thread_commands(mach_port_t task_port, struct thread_command_full_64 **thread_commands, natural_t *thread_count) +{ kern_return_t kr = KERN_SUCCESS; thread_act_array_t thread_info; struct thread_command_full_64 *threads = NULL; kr = task_threads(task_port, &thread_info, thread_count); - if (kr != KERN_SUCCESS) { + if (KERN_SUCCESS != kr) { mach_error("task_thread failed with: ", kr); return kr; } @@ -177,25 +222,28 @@ static int list_thread_commands(mach_port_t task_port, struct thread_command_ful goto done; } - for (int i = 0; i < *thread_count; i++) { + for (natural_t i = 0; i < *thread_count; i++) { uint32_t state_int_count; threads[i].cmd = LC_THREAD; threads[i].cmdsize = 8; state_int_count = x86_THREAD_STATE_COUNT; - kr = thread_get_state(thread_info[i], x86_THREAD_STATE, (thread_state_t)&threads[i].thread_state, &state_int_count); - if (kr != KERN_SUCCESS) { + kr = thread_get_state(thread_info[i], x86_THREAD_STATE, + (thread_state_t)&threads[i].thread_state, &state_int_count); + if (KERN_SUCCESS != kr) { goto done; } threads[i].cmdsize += state_int_count * 4; state_int_count = x86_FLOAT_STATE_COUNT; - kr = thread_get_state(thread_info[i], x86_FLOAT_STATE, (thread_state_t)&threads[i].float_state, &state_int_count); - if (kr != KERN_SUCCESS) { + kr = thread_get_state(thread_info[i], x86_FLOAT_STATE, + (thread_state_t)&threads[i].float_state, &state_int_count); + if (KERN_SUCCESS != kr) { goto done; } threads[i].cmdsize += state_int_count * 4; state_int_count = x86_EXCEPTION_STATE_COUNT; - kr = thread_get_state(thread_info[i], x86_EXCEPTION_STATE, (thread_state_t)&threads[i].exceptions, &state_int_count); - if (kr != KERN_SUCCESS) { + kr = thread_get_state(thread_info[i], x86_EXCEPTION_STATE, + (thread_state_t)&threads[i].exceptions, &state_int_count); + if (KERN_SUCCESS != kr) { goto done; } threads[i].cmdsize += state_int_count * 4; @@ -205,14 +253,15 @@ static int list_thread_commands(mach_port_t task_port, struct thread_command_ful } done: - for (int i = 0; i < *thread_count; i++) { + for (natural_t i = 0; i < *thread_count; i++) { mach_port_deallocate(mach_task_self(), thread_info[i]); } mach_vm_deallocate(task_port, (mach_vm_address_t)thread_info, (*thread_count) * sizeof(thread_act_t)); return kr; } -static int list_segment_commands(mach_port_t task_port, struct segment_command_64 **segment_commands, natural_t *segment_count) { +static kern_return_t list_segment_commands(mach_port_t task_port, struct segment_command_64 **segment_commands, natural_t *segment_count) +{ kern_return_t kr = KERN_SUCCESS; struct segment_command_64 *segments = NULL; struct segment_command_64 *tmp_segments = NULL; @@ -230,10 +279,10 @@ static int list_segment_commands(mach_port_t task_port, struct segment_command_6 vm_offset_t data_read; mach_msg_type_number_t data_size; - if ((*segment_count) >= segment_array_size) { + if (*segment_count >= segment_array_size) { segment_array_size += segment_array_size / 2; tmp_segments = calloc(segment_array_size, sizeof(struct segment_command_64)); - memcpy(tmp_segments, segments, (*segment_count) * sizeof(struct segment_command_64)); + memcpy(tmp_segments, segments, *segment_count * sizeof(struct segment_command_64)); free(segments); segments = tmp_segments; tmp_segments = NULL; @@ -241,57 +290,57 @@ static int list_segment_commands(mach_port_t task_port, struct segment_command_6 kr = vm_region_recurse_64(task_port, &address, &size, &depth, (vm_region_info_t)&vm_info, &info_count); - if (kr) { /* end of memory segments */ + if (KERN_SUCCESS != kr) { /* end of memory segments or actual error */ break; } if (vm_info.is_submap) { - depth++; + depth += 1; continue; } kr = mach_vm_read(task_port, address, size, &data_read, &data_size); - if (kr == KERN_SUCCESS) { + if (KERN_SUCCESS == kr) { mach_vm_deallocate(task_port, data_read, data_size); /* if we get the same region as previous, update previous region instead of creating a new one */ - if ((*segment_count > 0) && (old_addr == address + segments[(*segment_count - 1)].vmsize)) { - (*segment_count)--; - if (size <= segments[(*segment_count)].vmsize) { - size = segments[(*segment_count)].vmsize; + if ((*segment_count > 0) && (old_addr == address + segments[*segment_count - 1].vmsize)) { + *segment_count -= 1; + if (size <= segments[*segment_count].vmsize) { + size = segments[*segment_count].vmsize; address += 1; /* so we don't endlessly loop on one region */ } } - segments[(*segment_count)].cmd = LC_SEGMENT_64; - segments[(*segment_count)].cmdsize = sizeof(struct segment_command_64); - segments[(*segment_count)].segname[0] = 0; - segments[(*segment_count)].vmaddr = address; - segments[(*segment_count)].vmsize = size; - segments[(*segment_count)].fileoff = 0; /* get correct offset while writing to file */ - segments[(*segment_count)].filesize = size; - segments[(*segment_count)].maxprot = vm_info.max_protection; - segments[(*segment_count)].initprot = vm_info.protection; - segments[(*segment_count)].nsects = 0; - - (*segment_count)++; + segments[*segment_count].cmd = LC_SEGMENT_64; + segments[*segment_count].cmdsize = sizeof(struct segment_command_64); + segments[*segment_count].segname[0] = 0; + segments[*segment_count].vmaddr = address; + segments[*segment_count].vmsize = size; + segments[*segment_count].fileoff = 0; /* get correct offset while writing to file */ + segments[*segment_count].filesize = size; + segments[*segment_count].maxprot = vm_info.max_protection; + segments[*segment_count].initprot = vm_info.protection; + segments[*segment_count].nsects = 0; + + *segment_count += 1; } address += size; } - if (kr == KERN_INVALID_ADDRESS) { /* from reaching end of memory in vm_region_recurse_64 */ + if (KERN_INVALID_ADDRESS == kr) { /* from reaching end of memory in vm_region_recurse_64 */ kr = KERN_SUCCESS; } - if (kr == KERN_SUCCESS) { - tmp_segments = calloc((*segment_count), sizeof(struct segment_command_64)); - memcpy(tmp_segments, segments, (*segment_count) * sizeof(struct segment_command_64)); + if (KERN_SUCCESS == kr) { + tmp_segments = calloc(*segment_count, sizeof(struct segment_command_64)); + memcpy(tmp_segments, segments, *segment_count * sizeof(struct segment_command_64)); free(segments); *segment_commands = tmp_segments; } else { - if (segments) { + if (NULL != segments) { free(segments); } - if(tmp_segments) { + if(NULL != tmp_segments) { free(tmp_segments); } } @@ -321,19 +370,20 @@ static int list_segment_commands(mach_port_t task_port, struct segment_command_6 uintptr_t omrdump_create(struct OMRPortLibrary *portLibrary, char *filename, char *dumpType, void *userData) { - pid_t parent_pid, child_pid; - char* lastSep = NULL; + pid_t parent_pid = 0; + pid_t child_pid = 0; + char *lastSep = NULL; kern_return_t kr = KERN_SUCCESS; mach_port_t pass_port = MACH_PORT_NULL; mach_port_t special_port = MACH_PORT_NULL; parent_pid = getpid(); /* set core name, defaults to "core.%u" if none given */ - if (!filename || ('\0' == filename[0])) { + if (NULL == filename || ('\0' == filename[0])) { snprintf(corefile_name, PATH_MAX, "core.%u", parent_pid); } else { lastSep = strrchr(filename, DIR_SEPARATOR); - if (lastSep != NULL) { + if (NULL != lastSep) { strncpy(corefile_name, lastSep + 1, PATH_MAX); } else { strncpy(corefile_name, filename, PATH_MAX); @@ -342,13 +392,13 @@ omrdump_create(struct OMRPortLibrary *portLibrary, char *filename, char *dumpTyp /* pass parent task port to child through special port inheritance */ kr = task_get_bootstrap_port(mach_task_self(), &special_port); - if(kr != KERN_SUCCESS) { + if(KERN_SUCCESS != kr) { mach_error("failed get special port:\n", kr); return kr; } pass_port = mach_task_self(); kr = task_set_bootstrap_port(mach_task_self(), pass_port); - if(kr != KERN_SUCCESS) { + if(KERN_SUCCESS != kr) { mach_error("failed set special port:\n", kr); return kr; } @@ -356,7 +406,7 @@ omrdump_create(struct OMRPortLibrary *portLibrary, char *filename, char *dumpTyp child_pid = fork(); if (0 == child_pid) { /* in child process */ kr = task_get_bootstrap_port(mach_task_self(), &pass_port); - if(kr != KERN_SUCCESS) { + if(KERN_SUCCESS != kr) { mach_error("failed get special port:\n", kr); return kr; } @@ -366,8 +416,7 @@ omrdump_create(struct OMRPortLibrary *portLibrary, char *filename, char *dumpTyp } else { /* in parent process */ waitpid(child_pid, NULL, 0); kr = task_set_bootstrap_port(mach_task_self(), special_port); - if(kr != KERN_SUCCESS) - { + if(KERN_SUCCESS != kr) { mach_error("failed set special port:\n", kr); return kr; }