Skip to content

Commit

Permalink
feat: Merging safe_free updates made internally
Browse files Browse the repository at this point in the history 
Merging in updates to how safe_free is used and adding in use of wrapper functions to allow for type enforcement from the compiler.
Initial conversion to safe_Free with C_CAST(void**) was found to introduce memory leaks in some cases where it converted a struct *** to void**, so this solution fixes those issues and means less casting in the code, but more wrapper functions to allow this to be used safely.

Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
  • Loading branch information
@vonericsen
vonericsen committed Aug 26, 2024
1 parent 7fa2e00 commit f912283
Show file tree
Hide file tree
Showing 23 changed files with 81 additions and 78 deletions.
13 changes: 10 additions & 3 deletions include/openseachest_util_options.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,12 @@ extern "C"
#include "common_types.h"
#include "opensea_common_version.h"
#include "opensea_operation_version.h"
#include "secure_file.h"
#include "secure_file.h"
#include "common_public.h"

//this is being defined for using bools with getopt since using a bool (1 byte typically) will cause stack corruption at runtime
//This type should only be used where a boolean is desired when using the getopt parser (which expects an int), otherwise bool will do just fine
#define getOptBool int
typedef int getOptBool;
#define goFalse 0
#define goTrue !goFalse
#define CURRENT_YEAR_LENGTH 5
Expand Down Expand Up @@ -85,6 +86,12 @@ extern "C"
#define DEVICE_LONG_OPT_STRING "device"
#define DEVICE_LONG_OPT { DEVICE_LONG_OPT_STRING, required_argument, M_NULLPTR, DEVICE_SHORT_OPT }

//NOTE: This is to clean up the single allocation in the utility layer before it exits - TJE
static M_INLINE void free_device_list(tDevice **list)
{
safe_Free(M_REINTERPRET_CAST(void**, list));
}

#define SHOW_HELP_FLAG showHelp
#define SHOW_HELP_VAR bool SHOW_HELP_FLAG = false;
#define HELP_SHORT_OPT 'h'
Expand Down Expand Up @@ -3626,7 +3633,7 @@ extern "C"
int parse_Device_Handle_Argument(char * optarg, bool *allDrives, bool *userHandleProvided, uint32_t *deviceCount, char ***handleList);

//this call is to free the entire list of handles since they are all dynamically allocated.
void free_Handle_List(char ***handleList, uint32_t listCount);
void free_Handle_List(char*** handleList, uint32_t listCount);

#if defined (ENABLE_CSMI)
void print_CSMI_Force_Flags_Help(bool shortHelp);
Expand Down
8 changes: 4 additions & 4 deletions src/openseachest_util_options.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ void openseachest_utility_Info(const char *utilityName, const char *buildVersion
}
printf(" Today: %s\tUser: %s\n", CURRENT_TIME_STRING, userName);
printf("==========================================================================================\n");
safe_Free(C_CAST(void**, &userName));
safe_Free(C_CAST(void**, &year));
safe_free(&userName);
safe_free(&year);
}

void utility_Full_Version_Info(const char *utilityName, const char *buildVersion, int seaCPublicMajorVersion, int seaCPublicMinorVersion, int seaCPublicPatchVersion, const char * openseaCommonVersion, const char * openseaOperationVersion)
Expand Down Expand Up @@ -3364,9 +3364,9 @@ void free_Handle_List(char ***handleList, uint32_t listCount)
{
for (uint32_t handleIter = 0; handleIter < listCount; ++handleIter)
{
safe_Free(C_CAST(void**, &(*handleList)[handleIter]));
safe_free(&(*handleList)[handleIter]);
}
safe_Free(C_CAST(void**, (*handleList)));
safe_free((*handleList));
}
}

Expand Down
2 changes: 1 addition & 1 deletion subprojects/opensea-common
Submodule opensea-common updated 25 files
+40 −16 Make/VS.2019/opensea-common/opensea-common/opensea-common.vcxproj
+45 −83 Make/VS.2022/opensea-common/opensea-common/opensea-common.vcxproj
+100 −10 Make/VS.2022/opensea-common/opensea-common/opensea-common.vcxproj.filters
+87 −57 Make/vmware/Makefile
+6 −6 include/bit_manip.h
+2 −0 include/code_attributes.h
+1 −0 include/io_utils.h
+271 −1 include/memory_safety.h
+7 −0 include/precision_timer.h
+1 −1 include/predef_env_detect.h
+1 −1 include/type_conversion.h
+1 −1 src/error_translation.c
+45 −43 src/io_utils.c
+0 −5 src/memory_safety.c
+2 −2 src/pattern_utils.c
+14 −14 src/posix_env_detect.c
+18 −18 src/posix_secure_file.c
+34 −34 src/secure_file.c
+1 −1 src/secured_env_vars.c
+3 −3 src/string_utils.c
+1 −1 src/time_utils.c
+4 −4 src/validate_format.c
+3 −3 src/windows_env_detect.c
+33 −23 src/windows_secure_file.c
+2 −2 src/windows_version_detect.c
2 changes: 1 addition & 1 deletion subprojects/opensea-operations
Submodule opensea-operations updated 35 files
+68 −53 Make/vmware/Makefile
+10 −0 include/defect.h
+5 −0 include/depopulate.h
+5 −0 include/drive_info.h
+5 −0 include/format.h
+19 −1 include/partition_info.h
+6 −0 include/power_control.h
+18 −0 include/reservations.h
+5 −0 include/sector_repair.h
+5 −0 include/set_max_lba.h
+8 −0 include/zoned_operations.h
+4 −4 src/ata_Security.c
+6 −6 src/buffer_test.c
+10 −10 src/defect.c
+4 −4 src/depopulate.c
+3 −3 src/device_statistics.c
+54 −54 src/drive_info.c
+5 −5 src/dst.c
+29 −29 src/farm_log.c
+5 −5 src/firmware_download.c
+8 −8 src/format.c
+50 −50 src/generic_tests.c
+4 −4 src/host_erase.c
+94 −94 src/logs.c
+21 −21 src/operations.c
+10 −7 src/partition_info.c
+31 −31 src/power_control.c
+11 −11 src/reservations.c
+14 −14 src/seagate_operations.c
+2 −2 src/sector_repair.c
+6 −6 src/set_max_lba.c
+14 −14 src/smart.c
+6 −6 src/trim_unmap.c
+7 −7 src/writesame.c
+2 −2 src/zoned_operations.c
2 changes: 1 addition & 1 deletion subprojects/opensea-transport
Submodule opensea-transport updated 36 files
+68 −53 Make/vmware/Makefile
+13 −0 include/common_public.h
+7 −0 include/scsi_helper.h
+2 −49 include/vm_nvme_lib.h
+5 −5 include/vm_nvme_mgmt.h
+8 −8 src/aix_helper.c
+4 −4 src/asmedia_nvme_helper.c
+28 −20 src/ata_helper.c
+4 −2 src/ata_legacy_cmds.c
+32 −20 src/cam_helper.c
+7 −7 src/ciss_helper.c
+5 −5 src/cmds.c
+24 −21 src/common_public.c
+28 −21 src/csmi_helper.c
+1 −1 src/csmi_legacy_pt_cdb_helper.c
+1 −1 src/cypress_legacy_helper.c
+33 −8 src/intel_rst_helper.c
+1 −1 src/nec_legacy_helper.c
+1 −5 src/nvme_cmds.c
+1 −1 src/nvme_helper.c
+4 −4 src/of_nvme_helper.c
+1 −1 src/prolific_legacy_helper.c
+1 −1 src/psp_legacy_helper.c
+9 −4 src/raid_scan_helper.c
+238 −148 src/sat_helper.c
+8 −5 src/scsi_cmds.c
+67 −49 src/scsi_helper.c
+63 −61 src/sg_helper.c
+41 −27 src/sntl_helper.c
+1 −1 src/ti_legacy_helper.c
+40 −35 src/uefi_helper.c
+14 −8 src/uscsi_helper.c
+301 −926 src/vm_helper.c
+423 −317 src/vm_nvme_lib.c
+6 −23 src/vm_nvme_mgmt_common.c
+143 −76 src/win_helper.c
16 changes: 7 additions & 9 deletions utils/C/openSeaChest/openSeaChest_Basics.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,7 @@ int main(int argc, char *argv[])
printf("Error Reading LBA %" PRIu64 " for display\n", DISPLAY_LBA_THE_LBA);
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &displaySector));
safe_free_aligned(&displaySector);
}

if (SPIN_DOWN_FLAG)
Expand Down Expand Up @@ -1600,7 +1600,7 @@ int main(int argc, char *argv[])
}
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &firmwareMem));
safe_free_aligned(&firmwareMem);
}
else
{
Expand Down Expand Up @@ -2147,7 +2147,7 @@ int main(int argc, char *argv[])
{
case SUCCESS:
scsiAtaInSync = is_Max_LBA_In_Sync_With_Adapter_Or_Driver(&deviceList[deviceIter], false);
fill_Drive_Info_Data(&deviceList[deviceIter]);//refresh stale data
fill_Drive_Info_Data(&deviceList[deviceIter]);
if (VERBOSITY_QUIET < toolVerbosity)
{
double mCapacity = 0;
Expand All @@ -2172,10 +2172,8 @@ int main(int argc, char *argv[])
if (!scsiAtaInSync)
{
printf("\nWARNING: The adapter/driver/bridge is not in sync with the capacity change!\n");
printf(" If using a drive managed erase, this will not be an issue since the\n");
printf(" drive firmware will handle this change properly.\n");
printf(" If performing a manual overwrite, such as --%s, then a power cycle\n", OVERWRITE_LONG_OPT_STRING);
printf(" is strongly recommended to make sure all writes complete without error.\n");
printf(" A reboot is strongly recommended to make sure the system works without\n");
printf(" errors with the drive at its new capacity.\n\n");
}
}
break;
Expand All @@ -2197,11 +2195,11 @@ int main(int argc, char *argv[])
}
if (RESTORE_MAX_LBA_FLAG)
{
bool scsiAtaInSync = false;
if (VERBOSITY_QUIET < toolVerbosity)
{
printf("Restoring max LBA\n");
}
bool scsiAtaInSync = false;
switch (set_Max_LBA_2(&deviceList[deviceIter], 0, true, CHANGE_ID_STRING_FLAG))
{
case SUCCESS:
Expand Down Expand Up @@ -2297,7 +2295,7 @@ int main(int argc, char *argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
36 changes: 17 additions & 19 deletions utils/C/openSeaChest/openSeaChest_Configure.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,7 @@ int main(int argc, char *argv[])
errorInCL = true;
}
}
safe_Free(C_CAST(void**, &dupoptarg));
safe_free(&dupoptarg);
if (errorInCL)
{
print_Error_In_Cmd_Line_Args(SCSI_MP_RESET_LONG_OPT_STRING, optarg);
Expand Down Expand Up @@ -761,7 +761,7 @@ int main(int argc, char *argv[])
errorInCL = true;
}
}
safe_Free(C_CAST(void**, &dupoptarg));
safe_free(&dupoptarg);
if (errorInCL)
{
print_Error_In_Cmd_Line_Args(SCSI_MP_RESTORE_LONG_OPT_STRING, optarg);
Expand Down Expand Up @@ -815,7 +815,7 @@ int main(int argc, char *argv[])
errorInCL = true;
}
}
safe_Free(C_CAST(void**, &dupoptarg));
safe_free(&dupoptarg);
if (errorInCL)
{
print_Error_In_Cmd_Line_Args(SCSI_MP_SAVE_LONG_OPT_STRING, optarg);
Expand Down Expand Up @@ -869,7 +869,7 @@ int main(int argc, char *argv[])
errorInCL = true;
}
}
safe_Free(C_CAST(void**, &dupoptarg));
safe_free(&dupoptarg);
if (errorInCL)
{
print_Error_In_Cmd_Line_Args(SCSI_SHOW_MP_LONG_OPT_STRING, optarg);
Expand Down Expand Up @@ -965,7 +965,7 @@ int main(int argc, char *argv[])
print_Error_In_Cmd_Line_Args(SCSI_SET_MP_LONG_OPT_STRING, optarg);
exit(UTIL_EXIT_ERROR_IN_COMMAND_LINE);
}
safe_Free(C_CAST(void**, &dupoptarg));
safe_free(&dupoptarg);
saveptr = M_NULLPTR;
rsize_t pageSPlen = safe_strlen(pageAndSubpage);
char *pagetoken = common_String_Token(pageAndSubpage, &pageSPlen, "-", &saveptr);
Expand Down Expand Up @@ -1105,7 +1105,7 @@ int main(int argc, char *argv[])
errorInCL = true;
}
}
safe_Free(C_CAST(void**, &dupoptarg));
safe_free(&dupoptarg);
if (errorInCL)
{
print_Error_In_Cmd_Line_Args(SCSI_RESET_LP_PAGE_LONG_OPT_STRING, optarg);
Expand Down Expand Up @@ -1302,14 +1302,14 @@ int main(int argc, char *argv[])
else
{
//error, unknown option
safe_Free(C_CAST(void**, &dcoDisableFeatList));
ATA_DCO_DISABLE_FEATURES = false;
safe_free(&dcoDisableFeatList);
ATA_DCO_DISABLE_FEATURES = false;
print_Error_In_Cmd_Line_Args(ATA_DCO_DISABLE_FEEATURES_LONG_OPT_STRING, optarg);
exit(UTIL_EXIT_ERROR_IN_COMMAND_LINE);
}
dcoFeatToken = common_String_Token(M_NULLPTR, &featlistlen, ",", &saveptr);
}
safe_Free(C_CAST(void**, &dcoDisableFeatList));
safe_free(&dcoDisableFeatList);
}
else
{
Expand Down Expand Up @@ -3394,7 +3394,7 @@ int main(int argc, char *argv[])
{
case SUCCESS:
scsiAtaInSync = is_Max_LBA_In_Sync_With_Adapter_Or_Driver(&deviceList[deviceIter], false);
fill_Drive_Info_Data(&deviceList[deviceIter]);//refresh stale data
fill_Drive_Info_Data(&deviceList[deviceIter]);
if (VERBOSITY_QUIET < toolVerbosity)
{
double mCapacity = 0;
Expand All @@ -3419,10 +3419,8 @@ int main(int argc, char *argv[])
if (!scsiAtaInSync)
{
printf("\nWARNING: The adapter/driver/bridge is not in sync with the capacity change!\n");
printf(" If using a drive managed erase, this will not be an issue since the\n");
printf(" drive firmware will handle this change properly.\n");
printf(" If performing a manual overwrite, such as --%s, then a power cycle\n", OVERWRITE_LONG_OPT_STRING);
printf(" is strongly recommended to make sure all writes complete without error.\n");
printf(" A reboot is strongly recommended to make sure the system works without\n");
printf(" errors with the drive at its new capacity.\n\n");
}
}
break;
Expand Down Expand Up @@ -4058,7 +4056,7 @@ int main(int argc, char *argv[])
}
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &modePageBuffer));
safe_free_aligned(&modePageBuffer);
}
else
{
Expand All @@ -4068,8 +4066,8 @@ int main(int argc, char *argv[])
}
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &modePageBuffer));
safe_Free(C_CAST(void**, &fileBuf));
safe_free_aligned(&modePageBuffer);
safe_free(&fileBuf);
}
else
{
Expand Down Expand Up @@ -4234,7 +4232,7 @@ int main(int argc, char *argv[])
}
exitCode = UTIL_EXIT_OPERATION_NOT_SUPPORTED;
}
safe_Free(C_CAST(void**, &rawmodePageBuffer));
safe_free(&rawmodePageBuffer);
}
else
{
Expand Down Expand Up @@ -4342,7 +4340,7 @@ int main(int argc, char *argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
4 changes: 2 additions & 2 deletions utils/C/openSeaChest/openSeaChest_Defect.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1175,7 +1175,7 @@ int main(int argc, char *argv[])
exitCode = UTIL_EXIT_OPERATION_FAILURE;
break;
}
safe_Free(C_CAST(void**, &pendingLBAs));
safe_free_pending_defect(&pendingLBAs);
}
else
{
Expand Down Expand Up @@ -1553,7 +1553,7 @@ int main(int argc, char *argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
10 changes: 5 additions & 5 deletions utils/C/openSeaChest/openSeaChest_Erase.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ int main(int argc, char *argv[])
else
{
char *colonLocation = strstr(optarg, ":") + 1;//adding 1 to offset just beyond the colon for parsing the remaining data
if (strncmp("file:", optarg, 5) == 0 && colonLocation)
if (colonLocation && strncmp("file:", optarg, 5) == 0)
{
fileExt allowedExt[] = {
{".bin", false},
Expand Down Expand Up @@ -833,7 +833,7 @@ int main(int argc, char *argv[])
exit(UTIL_EXIT_CANNOT_OPEN_FILE);
}
}
else if (strncmp("increment:", optarg, 10) == 0 && colonLocation)
else if (colonLocation && strncmp("increment:", optarg, 10) == 0)
{
uint8_t incrementStart = 0;
if (get_And_Validate_Integer_Input_Uint8(colonLocation, M_NULLPTR, ALLOW_UNIT_NONE, &incrementStart))
Expand All @@ -846,7 +846,7 @@ int main(int argc, char *argv[])
exit(UTIL_EXIT_ERROR_IN_COMMAND_LINE);
}
}
else if (strncmp("repeat:", optarg, 7) == 0 && colonLocation)
else if (colonLocation && strncmp("repeat:", optarg, 7) == 0)
{
//if final character is a lower case h, it's an hex pattern
if (colonLocation[safe_strlen(colonLocation) - 1] == 'h' && safe_strlen(colonLocation) == 9)
Expand Down Expand Up @@ -1682,7 +1682,7 @@ int main(int argc, char *argv[])
printf("Error Reading LBA %" PRIu64 " for display\n", DISPLAY_LBA_THE_LBA);
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &displaySector));
safe_free_aligned(&displaySector);
}

//check for fastest erase flag first since it may be used to set other flags for other erase methods
Expand Down Expand Up @@ -3264,7 +3264,7 @@ int main(int argc, char *argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
4 changes: 2 additions & 2 deletions utils/C/openSeaChest/openSeaChest_Firmware.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1163,7 +1163,7 @@ int main(int argc, char* argv[])
}
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &firmwareMem));
safe_free_aligned(&firmwareMem);
}
else
{
Expand Down Expand Up @@ -1314,7 +1314,7 @@ int main(int argc, char* argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
6 changes: 3 additions & 3 deletions utils/C/openSeaChest/openSeaChest_Format.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1221,7 +1221,7 @@ int main(int argc, char *argv[])
printf("Error Reading LBA %" PRIu64 " for display\n", DISPLAY_LBA_THE_LBA);
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &displaySector));
safe_free_aligned(&displaySector);
}

if (SHOW_SUPPORTED_FORMATS_FLAG)
Expand Down Expand Up @@ -1260,7 +1260,7 @@ int main(int argc, char *argv[])
exitCode = UTIL_EXIT_OPERATION_FAILURE;
break;
}
safe_Free(C_CAST(void**, &formats));
safe_free_supported_formats(&formats);
}
else
{
Expand Down Expand Up @@ -1943,7 +1943,7 @@ int main(int argc, char *argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
4 changes: 2 additions & 2 deletions utils/C/openSeaChest/openSeaChest_GenericTests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1133,7 +1133,7 @@ int main(int argc, char *argv[])
printf("Error Reading LBA %" PRIu64 " for display\n", DISPLAY_LBA_THE_LBA);
exitCode = UTIL_EXIT_OPERATION_FAILURE;
}
safe_Free_aligned(C_CAST(void**, &displaySector));
safe_free_aligned(&displaySector);
}

if (BUFFER_TEST_FLAG)
Expand Down Expand Up @@ -1562,7 +1562,7 @@ int main(int argc, char *argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
2 changes: 1 addition & 1 deletion utils/C/openSeaChest/openSeaChest_Info.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ int main(int argc, char *argv[])
//At this point, close the device handle since it is no longer needed. Do not put any further IO below this.
close_Device(&deviceList[deviceIter]);
}
safe_Free(C_CAST(void**, &DEVICE_LIST));
free_device_list(&DEVICE_LIST);
exit(exitCode);
}

Expand Down
Loading

0 comments on commit f912283

Please sign in to comment.