From 7900660d3a12573db8a68bc7df46d7552dae54d1 Mon Sep 17 00:00:00 2001 From: David Steele Date: Mon, 25 Apr 2022 11:47:43 -0400 Subject: [PATCH] Add strLstNewFmt(). Simplifies adding a formatted string to a list and removes a common cause of leaks. --- src/command/archive/get/file.c | 8 +++--- src/command/archive/push/file.c | 13 ++++------ src/command/archive/push/push.c | 7 +++--- src/command/backup/backup.c | 2 +- src/command/command.c | 8 +++--- src/command/restore/restore.c | 2 +- src/common/type/stringList.c | 33 +++++++++++++++++++++++++ src/common/type/stringList.h | 1 + src/config/exec.c | 16 +++++------- src/config/parse.c | 4 +-- src/postgres/interface.c | 4 +-- src/protocol/helper.c | 10 +++----- test/src/common/harnessConfig.c | 8 +++--- test/src/common/harnessStorage.c | 6 ++--- test/src/module/command/repoTest.c | 10 +++----- test/src/module/common/typeStringTest.c | 4 +-- test/src/module/config/parseTest.c | 2 +- 17 files changed, 78 insertions(+), 60 deletions(-) diff --git a/src/command/archive/get/file.c b/src/command/archive/get/file.c index 27409a798..a5f9fb5a5 100644 --- a/src/command/archive/get/file.c +++ b/src/command/archive/get/file.c @@ -89,11 +89,9 @@ ArchiveGetFileResult archiveGetFile( { MEM_CONTEXT_PRIOR_BEGIN() { - strLstAdd( - result.warnList, - strNewFmt( - "%s: %s [%s] %s", cfgOptionGroupName(cfgOptGrpRepo, actual->repoIdx), strZ(actual->file), - errorTypeName(errorType()), errorMessage())); + strLstAddFmt( + result.warnList, "%s: %s [%s] %s", cfgOptionGroupName(cfgOptGrpRepo, actual->repoIdx), strZ(actual->file), + errorTypeName(errorType()), errorMessage()); } MEM_CONTEXT_PRIOR_END(); } diff --git a/src/command/archive/push/file.c b/src/command/archive/push/file.c index 309a2cbeb..8f345d440 100644 --- a/src/command/archive/push/file.c +++ b/src/command/archive/push/file.c @@ -38,9 +38,7 @@ archivePushErrorAdd(StringList *errorList, unsigned int repoIdx) FUNCTION_TEST_PARAM(UINT, repoIdx); FUNCTION_TEST_END(); - strLstAdd( - errorList, - strNewFmt("%s: [%s] %s", cfgOptionGroupName(cfgOptGrpRepo, repoIdx), errorTypeName(errorType()), errorMessage())); + strLstAddFmt(errorList, "%s: [%s] %s", cfgOptionGroupName(cfgOptGrpRepo, repoIdx), errorTypeName(errorType()), errorMessage()); FUNCTION_TEST_RETURN_VOID(); } @@ -201,12 +199,11 @@ archivePushFile( MEM_CONTEXT_PRIOR_BEGIN() { // Add warning to the result that will be returned to the main process - strLstAdd( + strLstAddFmt( result.warnList, - strNewFmt( - "WAL file '%s' already exists in the %s archive with the same checksum" - "\nHINT: this is valid in some recovery scenarios but may also indicate a problem.", - strZ(archiveFile), cfgOptionGroupName(cfgOptGrpRepo, repoData->repoIdx))); + "WAL file '%s' already exists in the %s archive with the same checksum" + "\nHINT: this is valid in some recovery scenarios but may also indicate a problem.", + strZ(archiveFile), cfgOptionGroupName(cfgOptGrpRepo, repoData->repoIdx)); } MEM_CONTEXT_PRIOR_END(); } diff --git a/src/command/archive/push/push.c b/src/command/archive/push/push.c index 45fd0759f..8dcf97027 100644 --- a/src/command/archive/push/push.c +++ b/src/command/archive/push/push.c @@ -276,10 +276,9 @@ archivePushCheck(bool pgPathSet) } CATCH_ANY() { - strLstAdd( - result.errorList, - strNewFmt( - "%s: [%s] %s", cfgOptionGroupName(cfgOptGrpRepo, repoIdx), errorTypeName(errorType()), errorMessage())); + strLstAddFmt( + result.errorList, "%s: [%s] %s", cfgOptionGroupName(cfgOptGrpRepo, repoIdx), errorTypeName(errorType()), + errorMessage()); } TRY_END(); } diff --git a/src/command/backup/backup.c b/src/command/backup/backup.c index eb8bf1198..e83895a92 100644 --- a/src/command/backup/backup.c +++ b/src/command/backup/backup.c @@ -1488,7 +1488,7 @@ backupProcessQueue(const BackupData *const backupData, Manifest *const manifest, const ManifestTarget *target = manifestTarget(manifest, targetIdx); if (target->tablespaceId != 0) - strLstAdd(targetList, strNewFmt("%s/", strZ(target->name))); + strLstAddFmt(targetList, "%s/", strZ(target->name)); } // Generate the processing queues (there is always at least one) diff --git a/src/command/command.c b/src/command/command.c index bba9d54f8..2afbdf6b1 100644 --- a/src/command/command.c +++ b/src/command/command.c @@ -114,11 +114,9 @@ cmdOption(void) for (unsigned int keyIdx = 0; keyIdx < varLstSize(keyList); keyIdx++) { - strLstAdd( - valueList, - strNewFmt( - "%s=%s", strZ(varStr(varLstGet(keyList, keyIdx))), - strZ(varStrForce(kvGet(optionKv, varLstGet(keyList, keyIdx)))))); + strLstAddFmt( + valueList, "%s=%s", strZ(varStr(varLstGet(keyList, keyIdx))), + strZ(varStrForce(kvGet(optionKv, varLstGet(keyList, keyIdx))))); } } // Generate values for list options diff --git a/src/command/restore/restore.c b/src/command/restore/restore.c index 5fa41ba44..cdac5152c 100644 --- a/src/command/restore/restore.c +++ b/src/command/restore/restore.c @@ -2010,7 +2010,7 @@ restoreProcessQueue(Manifest *manifest, List **queueList) const ManifestTarget *target = manifestTarget(manifest, targetIdx); if (target->tablespaceId != 0) - strLstAdd(targetList, strNewFmt("%s/", strZ(target->name))); + strLstAddFmt(targetList, "%s/", strZ(target->name)); } // Generate the processing queues diff --git a/src/common/type/stringList.c b/src/common/type/stringList.c index 96dbcaed1..5814cc456 100644 --- a/src/common/type/stringList.c +++ b/src/common/type/stringList.c @@ -151,6 +151,39 @@ strLstAdd(StringList *this, const String *string) FUNCTION_TEST_RETURN(STRING, result); } +String * +strLstAddFmt(StringList *const this, const char *const format, ...) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(STRING_LIST, this); + FUNCTION_TEST_PARAM(STRINGZ, format); + FUNCTION_TEST_END(); + + // Determine how long the allocated string needs to be + va_list argumentList; + va_start(argumentList, format); + const size_t size = (size_t)vsnprintf(NULL, 0, format, argumentList); + va_end(argumentList); + + // Format string + va_start(argumentList, format); + char *const buffer = memNew(size + 1); + vsnprintf(buffer, size + 1, format, argumentList); + va_end(argumentList); + + String *result = NULL; + + MEM_CONTEXT_BEGIN(lstMemContext((List *)this)) + { + result = strLstAddInternal(this, strNewZN(buffer, size)); + } + MEM_CONTEXT_END(); + + memFree(buffer); + + FUNCTION_TEST_RETURN(STRING, result); +} + String * strLstAddIfMissing(StringList *this, const String *string) { diff --git a/src/common/type/stringList.h b/src/common/type/stringList.h index 4ebdc0c68..3288c60a0 100644 --- a/src/common/type/stringList.h +++ b/src/common/type/stringList.h @@ -68,6 +68,7 @@ Functions ***********************************************************************************************************************************/ // Add String to the list String *strLstAdd(StringList *this, const String *string); +String *strLstAddFmt(StringList *this, const char *format, ...) __attribute__((format(printf, 2, 3))); String *strLstAddZ(StringList *this, const char *string); String *strLstAddIfMissing(StringList *this, const String *string); diff --git a/src/config/exec.c b/src/config/exec.c index 2d356be5e..8f6a30713 100644 --- a/src/config/exec.c +++ b/src/config/exec.c @@ -77,15 +77,13 @@ cfgExecParam(ConfigCommand commandId, ConfigCommandRole commandRoleId, const Key // If the option was reset if (value == NULL && cfgOptionValid(optionId) && cfgOptionIdxReset(optionId, optionIdx)) { - strLstAdd(result, strNewFmt("--reset-%s", cfgOptionIdxName(optionId, optionIdx))); + strLstAddFmt(result, "--reset-%s", cfgOptionIdxName(optionId, optionIdx)); } // Else format the value if found, even if the option is not valid for the command else if (value != NULL && (!local || exists || cfgOptionIdxSource(optionId, optionIdx) == cfgSourceParam)) { if (varType(value) == varTypeBool) - { - strLstAdd(result, strNewFmt("--%s%s", varBool(value) ? "" : "no-", cfgOptionIdxName(optionId, optionIdx))); - } + strLstAddFmt(result, "--%s%s", varBool(value) ? "" : "no-", cfgOptionIdxName(optionId, optionIdx)); else { StringList *valueList = NULL; @@ -99,11 +97,9 @@ cfgExecParam(ConfigCommand commandId, ConfigCommandRole commandRoleId, const Key for (unsigned int keyIdx = 0; keyIdx < varLstSize(keyList); keyIdx++) { - strLstAdd( - valueList, - strNewFmt( - "%s=%s", strZ(varStr(varLstGet(keyList, keyIdx))), - strZ(varStrForce(kvGet(optionKv, varLstGet(keyList, keyIdx)))))); + strLstAddFmt( + valueList, "%s=%s", strZ(varStr(varLstGet(keyList, keyIdx))), + strZ(varStrForce(kvGet(optionKv, varLstGet(keyList, keyIdx))))); } } else if (varType(value) == varTypeVariantList) @@ -129,7 +125,7 @@ cfgExecParam(ConfigCommand commandId, ConfigCommandRole commandRoleId, const Key if (quote && strchr(strZ(value), ' ') != NULL) value = strNewFmt("\"%s\"", strZ(value)); - strLstAdd(result, strNewFmt("--%s=%s", cfgOptionIdxName(optionId, optionIdx), strZ(value))); + strLstAddFmt(result, "--%s=%s", cfgOptionIdxName(optionId, optionIdx), strZ(value)); } } } diff --git a/src/config/parse.c b/src/config/parse.c index 796120e4c..eb80e9c3b 100644 --- a/src/config/parse.c +++ b/src/config/parse.c @@ -1748,11 +1748,11 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis if (stanza != NULL) { - strLstAdd(sectionList, strNewFmt("%s:%s", strZ(stanza), cfgParseCommandName(config->command))); + strLstAddFmt(sectionList, "%s:%s", strZ(stanza), cfgParseCommandName(config->command)); strLstAdd(sectionList, stanza); } - strLstAdd(sectionList, strNewFmt(CFGDEF_SECTION_GLOBAL ":%s", cfgParseCommandName(config->command))); + strLstAddFmt(sectionList, CFGDEF_SECTION_GLOBAL ":%s", cfgParseCommandName(config->command)); strLstAddZ(sectionList, CFGDEF_SECTION_GLOBAL); // Loop through sections to search for options diff --git a/src/postgres/interface.c b/src/postgres/interface.c index 8d141e4c5..1fa6e80c1 100644 --- a/src/postgres/interface.c +++ b/src/postgres/interface.c @@ -572,7 +572,7 @@ pgLsnRangeToWalSegmentList( unsigned int minorPerMajor = 0xFFFFFFFF / walSegmentSize; // Create list - strLstAdd(result, strNewFmt("%08X%08X%08X", timeline, startMajor, startMinor)); + strLstAddFmt(result, "%08X%08X%08X", timeline, startMajor, startMinor); while (!(startMajor == stopMajor && startMinor == stopMinor)) { @@ -584,7 +584,7 @@ pgLsnRangeToWalSegmentList( startMinor = 0; } - strLstAdd(result, strNewFmt("%08X%08X%08X", timeline, startMajor, startMinor)); + strLstAddFmt(result, "%08X%08X%08X", timeline, startMajor, startMinor); } strLstMove(result, memContextPrior()); diff --git a/src/protocol/helper.c b/src/protocol/helper.c index 5adbd7aed..5d75ae7ff 100644 --- a/src/protocol/helper.c +++ b/src/protocol/helper.c @@ -597,15 +597,13 @@ protocolRemoteParamSsh(const ProtocolStorageType protocolStorageType, const unsi if (cfgOptionIdxTest(optHostPort, hostIdx)) { strLstAddZ(result, "-p"); - strLstAdd(result, strNewFmt("%u", cfgOptionIdxUInt(optHostPort, hostIdx))); + strLstAddFmt(result, "%u", cfgOptionIdxUInt(optHostPort, hostIdx)); } // Append user/host - strLstAdd( - result, - strNewFmt( - "%s@%s", strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHostUser : cfgOptPgHostUser, hostIdx)), - strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHost : cfgOptPgHost, hostIdx)))); + strLstAddFmt( + result, "%s@%s", strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHostUser : cfgOptPgHostUser, hostIdx)), + strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHost : cfgOptPgHost, hostIdx))); // Add remote command and parameters StringList *paramList = protocolRemoteParam(protocolStorageType, hostIdx); diff --git a/test/src/common/harnessConfig.c b/test/src/common/harnessConfig.c index ad5d65b43..1694e4e90 100644 --- a/test/src/common/harnessConfig.c +++ b/test/src/common/harnessConfig.c @@ -144,7 +144,7 @@ hrnCfgArgRawZ(StringList *argList, ConfigOption optionId, const char *value) void hrnCfgArgKeyRawZ(StringList *argList, ConfigOption optionId, unsigned optionKey, const char *value) { - strLstAdd(argList, strNewFmt("--%s=%s", cfgParseOptionKeyIdxName(optionId, optionKey - 1), value)); + strLstAddFmt(argList, "--%s=%s", cfgParseOptionKeyIdxName(optionId, optionKey - 1), value); } void @@ -171,7 +171,7 @@ hrnCfgArgRawBool(StringList *argList, ConfigOption optionId, bool value) void hrnCfgArgKeyRawBool(StringList *argList, ConfigOption optionId, unsigned optionKey, bool value) { - strLstAdd(argList, strNewFmt("--%s%s", value ? "" : "no-", cfgParseOptionKeyIdxName(optionId, optionKey - 1))); + strLstAddFmt(argList, "--%s%s", value ? "" : "no-", cfgParseOptionKeyIdxName(optionId, optionKey - 1)); } void @@ -183,7 +183,7 @@ hrnCfgArgRawNegate(StringList *argList, ConfigOption optionId) void hrnCfgArgKeyRawNegate(StringList *argList, ConfigOption optionId, unsigned optionKey) { - strLstAdd(argList, strNewFmt("--no-%s", cfgParseOptionKeyIdxName(optionId, optionKey - 1))); + strLstAddFmt(argList, "--no-%s", cfgParseOptionKeyIdxName(optionId, optionKey - 1)); } void @@ -195,7 +195,7 @@ hrnCfgArgRawReset(StringList *argList, ConfigOption optionId) void hrnCfgArgKeyRawReset(StringList *argList, ConfigOption optionId, unsigned optionKey) { - strLstAdd(argList, strNewFmt("--reset-%s", cfgParseOptionKeyIdxName(optionId, optionKey - 1))); + strLstAddFmt(argList, "--reset-%s", cfgParseOptionKeyIdxName(optionId, optionKey - 1)); } /**********************************************************************************************************************************/ diff --git a/test/src/common/harnessStorage.c b/test/src/common/harnessStorage.c index 81daa1242..5bcc57c4d 100644 --- a/test/src/common/harnessStorage.c +++ b/test/src/common/harnessStorage.c @@ -324,15 +324,15 @@ hrnStorageList(const Storage *const storage, const char *const path, const char break; case storageTypeLink: - strLstAdd(listStr, strNewFmt("%s>", strZ(info->name))); + strLstAddFmt(listStr, "%s>", strZ(info->name)); break; case storageTypePath: - strLstAdd(listStr, strNewFmt("%s/", strZ(info->name))); + strLstAddFmt(listStr, "%s/", strZ(info->name)); break; case storageTypeSpecial: - strLstAdd(listStr, strNewFmt("%s*", strZ(info->name))); + strLstAddFmt(listStr, "%s*", strZ(info->name)); break; } } diff --git a/test/src/module/command/repoTest.c b/test/src/module/command/repoTest.c index 9bdc4e3a2..d90486614 100644 --- a/test/src/module/command/repoTest.c +++ b/test/src/module/command/repoTest.c @@ -528,7 +528,7 @@ testRun(void) argList = strLstNew(); hrnCfgArgRawZ(argList, cfgOptRepoPath, "/"); - strLstAdd(argList, strNewFmt(TEST_PATH "/repo/%s", strZ(fileName))); + strLstAddFmt(argList, TEST_PATH "/repo/%s", strZ(fileName)); HRN_CFG_LOAD(cfgCmdRepoGet, argList); writeBuffer = bufNew(0); @@ -701,11 +701,9 @@ testRun(void) argList = strLstNew(); hrnCfgArgRawZ(argList, cfgOptRepoPath, TEST_PATH "/repo"); hrnCfgArgRawStrId(argList, cfgOptRepoCipherType, cipherTypeAes256Cbc); - strLstAdd( - argList, - strNewFmt( - "%s/repo/" STORAGE_PATH_ARCHIVE "/test/12-1/000000010000000100000001-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - TEST_PATH)); + strLstAddFmt( + argList, "%s/repo/" STORAGE_PATH_ARCHIVE "/test/12-1/000000010000000100000001-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + TEST_PATH); HRN_CFG_LOAD(cfgCmdRepoGet, argList); writeBuffer = bufNew(0); diff --git a/test/src/module/common/typeStringTest.c b/test/src/module/common/typeStringTest.c index b33a9f654..8f319e2cb 100644 --- a/test/src/module/common/typeStringTest.c +++ b/test/src/module/common/typeStringTest.c @@ -341,7 +341,7 @@ testRun(void) else { TEST_RESULT_STR( - strLstAdd(list, strNewFmt("STR%02d", listIdx)), strNewFmt("STR%02d", listIdx), + strLstAddFmt(list, "STR%02d", listIdx), strNewFmt("STR%02d", listIdx), strZ(strNewFmt("add item %d", listIdx))); } } @@ -418,7 +418,7 @@ testRun(void) if (listIdx == 0) strLstAdd(list, NULL); else - strLstAdd(list, strNewFmt("STR%02d", listIdx)); + strLstAddFmt(list, "STR%02d", listIdx); } // Check pointer diff --git a/test/src/module/config/parseTest.c b/test/src/module/config/parseTest.c index 12e4e18dc..b91976a87 100644 --- a/test/src/module/config/parseTest.c +++ b/test/src/module/config/parseTest.c @@ -781,7 +781,7 @@ testRun(void) argList = strLstNew(); strLstAddZ(argList, TEST_BACKREST_EXE); - strLstAdd(argList, strNewFmt("--%s", optionMax)); + strLstAddFmt(argList, "--%s", optionMax); TEST_ERROR_FMT( configParse(storageTest, strLstSize(argList), strLstPtr(argList), false), OptionInvalidError, "option '%s' exceeds maximum size of 64", optionMax);