Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacing FILENAME_MAX in runtime-directory #17059

Closed
wants to merge 6 commits into from
Closed

Replacing FILENAME_MAX in runtime-directory #17059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d841d9
Reduce-FILENAME_MAX-in-runtime-directory
rchinmay Jan 30, 2021
bba606e
Deleting Files added by mistake
rchinmay Jan 30, 2021
2e63718
Change-to-chpl_mem_-functions
rchinmay Feb 3, 2021
84b75d6
Changing free to chpl_mem_free functions
rchinmay Feb 3, 2021
820aff0
Added CHPL_RT_MD_FILENAME enumeration
rchinmay Feb 4, 2021
021170e
Review Changes
rchinmay Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions runtime/include/chpl-mem-desc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ extern "C" {
m(SET_WIDE_STRING, "set wide string", true ), \
m(GET_WIDE_STRING, "get wide string", true ), \
m(COMMAND_BUFFER, "command buffer", true ), \
m(FILENAME, "filename string", true ), \
m(COMM_UTIL, "comm layer utility space", false), \
m(COMM_XMIT_RCV_BUF, "comm layer transmit/receive buffer", false), \
m(COMM_FRK_SND_INFO, "comm layer sent remote fork info", false), \
Expand Down
16 changes: 12 additions & 4 deletions runtime/src/launch/pbs-aprun/launch-pbs-aprun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ static char* walltime = NULL;
static char* queue = NULL;
static int generate_qsub_script = 0;

static char expectFilename[FILENAME_MAX];
static char* expectFilename=NULL;

extern int fileno(FILE *stream);

Expand Down Expand Up @@ -243,6 +243,10 @@ static char** chpl_launch_create_argv(int argc, char* argv[],
} else {
mypid = 0;
}
expectFilename=(char *)chpl_mem_allocMany((strlen(baseExpectFilename) +
snprintf(NULL, 0, "%d", (int)mypid)
+ 1),
sizeof(char), CHPL_RT_MD_FILENAME, -1, 0);
snprintf(expectFilename, FILENAME_MAX, "%s%d",
baseExpectFilename, (int)mypid);

Expand Down Expand Up @@ -393,10 +397,14 @@ static void genQsubScript(int argc, char *argv[], int numLocales) {
static void chpl_launch_cleanup(void) {
if (!debug) {
if (unlink(expectFilename)) {
char msg[FILENAME_MAX + 35];
snprintf(msg, FILENAME_MAX + 35, "Error removing temporary file '%s': %s",
char *format="Error removing temporary file '%s': %s";
int msgLen=strlen(format) + strlen(expectFilename) + strlen(strerror(errno)
char* msg=(char *)chpl_mem_allocMany(msgLen, sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
snprintf(msg, msgLen, "Error removing temporary file '%s': %s",
expectFilename, strerror(errno));
chpl_warning(msg, 0, 0);
chpl_mem_free(msg);
}
}
}
Expand All @@ -416,7 +424,7 @@ int chpl_launch(int argc, char* argv[], int32_t numLocales) {
argv[0]);
chpl_launch_cleanup();
}

chpl_mem_free(expectFilename);
return retcode;
}

Expand Down
20 changes: 17 additions & 3 deletions runtime/src/launch/pbs-gasnetrun_ibv/launch-pbs-gasnetrun_ibv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@
#define baseExpectFilename ".chpl-expect-"
#define baseSysFilename ".chpl-sys-"

char pbsFilename[FILENAME_MAX];
char expectFilename[FILENAME_MAX];
char sysFilename[FILENAME_MAX];
char* pbsFilename=NULL;
char* expectFilename=NULL;
char* sysFilename=NULL;

/* copies of binary to run per node */
#define procsPerNode 1
Expand Down Expand Up @@ -170,6 +170,17 @@ static char* chpl_launch_create_command(int argc, char* argv[],
#else
mypid = 0;
#endif
sysFilename=(char *)chpl_mem_allocMany((strlen(baseSysFilename) +
snprintf(NULL, 0, "%d", (int)mypid) + 1),
Copy link
Member

Choose a reason for hiding this comment

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

I had not seen this pattern before—clever. Given that this same expression appears three times in a row, how about storing it into an int and then re-using that int in these three malloc calls. I.e., something like:

int mypidBuffLen = snprintf(NULL, 0, "%d", (int)mypid);

sizeof(char), CHPL_RT_MD_FILENAME, -1, 0);
expectFilename=(char *)chpl_mem_allocMany((strlen(baseExpectFilename) +
snprintf(NULL, 0, "%d", (int)mypid)
+ 1), sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
pbsFilename=(char *)chpl_mem_allocMany((strlen(basePBSFilename) +
snprintf(NULL, 0, "%d", (int)mypid) +
1), sizeof(char), CHPL_RT_MD_FILENAME,
-1, 0);
sprintf(sysFilename, "%s%d", baseSysFilename, (int)mypid);
sprintf(expectFilename, "%s%d", baseExpectFilename, (int)mypid);
sprintf(pbsFilename, "%s%d", basePBSFilename, (int)mypid);
Expand Down Expand Up @@ -235,6 +246,9 @@ static void chpl_launch_cleanup(void) {
sprintf(command, "rm %s", sysFilename);
system(command);
#endif
chpl_mem_free(pbsFilename);
chpl_mem_free(expectFilename);
chpl_mem_free(sysFilename);
}


Expand Down
27 changes: 19 additions & 8 deletions runtime/src/launch/slurm-gasnetrun_common/slurm-gasnetrun_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ static char* debug = NULL;
static char* walltime = NULL;
static char* partition = NULL;
static char* exclude = NULL;
char slurmFilename[FILENAME_MAX];
char* slurmFilename = NULL;

/* copies of binary to run per node */
#define procsPerNode 1
Expand Down Expand Up @@ -178,8 +178,8 @@ static char* chpl_launch_create_command(int argc, char* argv[],
int32_t numLocales) {
int i;
int size;
char baseCommand[2*FILENAME_MAX];
char envProp[2*FILENAME_MAX];
char* baseCommand = NULL;
char* envProp = NULL;
char* command;
FILE* slurmFile;
char* projectString = getenv(launcherAccountEnvvar);
Expand Down Expand Up @@ -233,6 +233,10 @@ static char* chpl_launch_create_command(int argc, char* argv[],
} else {
mypid = getpid();
}
slurmFilename=(char *)chpl_mem_allocMany((strlen(baseSBATCHFilename) +
snprintf(NULL, 0, "%d", (int)mypid)
+ 1), sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
sprintf(slurmFilename, "%s%d", baseSBATCHFilename, (int)mypid);

if (getenv("CHPL_LAUNCHER_USE_SBATCH") != NULL) {
Expand Down Expand Up @@ -268,8 +272,11 @@ static char* chpl_launch_create_command(int argc, char* argv[],

fclose(slurmFile);
chmod(slurmFilename, 0755);

sprintf(baseCommand, "sbatch %s\n", slurmFilename);
char* format="sbatch %s\n";
int baseCommandLen=strlen(slurmFilename) + strlen(format);
baseCommand=(char *)chpl_mem_allocMany(baseCommandLen), sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
sprintf(baseCommand, format, slurmFilename);
} else {
char iCom[2*FILENAME_MAX-10];
int len = 0;
Expand All @@ -296,16 +303,19 @@ static char* chpl_launch_create_command(int argc, char* argv[],
for (i=1; i<argc; i++) {
len += sprintf(iCom+len, " %s", argv[i]);
}

sprintf(baseCommand, "salloc %s", iCom);
char* format="salloc %s";
int baseCommandLen = strlen(format) + len;
baseCommand=(char *)chpl_mem_allocMany(baseCommandLen), sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
sprintf(baseCommand, format, iCom);
}

size = strlen(baseCommand) + 1;

command = chpl_mem_allocMany(size, sizeof(char), CHPL_RT_MD_COMMAND_BUFFER, -1, 0);

sprintf(command, "%s", baseCommand);

chpl_mem_free(baseCommand);
if (strlen(command)+1 > size) {
chpl_internal_error("buffer overflow");
}
Expand All @@ -330,6 +340,7 @@ int chpl_launch(int argc, char* argv[], int32_t numLocales) {
retcode = chpl_launch_using_system(chpl_launch_create_command(argc, argv, numLocales),
argv[0]);
chpl_launch_cleanup();
chpl_mem_free(slurmFilename);
return retcode;
}

Expand Down
65 changes: 51 additions & 14 deletions runtime/src/launch/slurm-srun/launch-slurm-srun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ static char* nodelist = NULL;
static char* partition = NULL;
static char* exclude = NULL;

char slurmFilename[FILENAME_MAX];
char* slurmFilename = NULL;

/* copies of binary to run per node */
#define procsPerNode 1
Expand Down Expand Up @@ -142,14 +142,13 @@ static int getCoresPerLocale(int nomultithread) {

return numCores;
}
#define MAX_COM_LEN (FILENAME_MAX + 128)
// create the command that will actually launch the program and
// create any files needed for the launch like the batch script
static char* chpl_launch_create_command(int argc, char* argv[],
int32_t numLocales) {
int i;
int size;
char baseCommand[MAX_COM_LEN];
char* baseCommand=NULL;
char* command;
FILE* slurmFile;
char* account = getenv("CHPL_LAUNCHER_ACCOUNT");
Expand Down Expand Up @@ -183,9 +182,9 @@ static char* chpl_launch_create_command(int argc, char* argv[],
// because they haven't been initialized yet
char* bufferStdout = getenv("CHPL_LAUNCHER_SLURM_BUFFER_STDOUT");
const char* tmpDir = getTmpDir();
char stdoutFile [MAX_COM_LEN];
char stdoutFileNoFmt [MAX_COM_LEN];
char tmpStdoutFileNoFmt [MAX_COM_LEN];
char* stdoutFile = NULL;
char* stdoutFileNoFmt = NULL;
char* tmpStdoutFileNoFmt = NULL;

// command line walltime takes precedence over env var
if (!walltime) {
Expand Down Expand Up @@ -244,6 +243,10 @@ static char* chpl_launch_create_command(int argc, char* argv[],

// if were running a batch job
if (getenv("CHPL_LAUNCHER_USE_SBATCH") != NULL || generate_sbatch_script) {
slurmFilename=(char *)chpl_mem_allocMany((strlen(baseSBATCHFilename) +
snprintf(NULL, 0, "%d", (int)mypid)
+ 1), sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
// set the sbatch filename
sprintf(slurmFilename, "%s%d", baseSBATCHFilename, (int)mypid);

Expand Down Expand Up @@ -305,14 +308,25 @@ static char* chpl_launch_create_command(int argc, char* argv[],
// set the output file name to either the user specified
// name or to the binaryName.<jobID>.out if none specified
if (outputfn != NULL) {
stdoutFile=(char *)chpl_mem_allocMany((strlen(outputfn) + 1),
sizeof(char), CHPL_RT_MD_FILENAME, -1, 0);
sprintf(stdoutFile, "%s", outputfn);
stdoutFileNoFmt=(char *)chpl_mem_allocMany((strlen(outputfn) + 1),
sizeof(char), CHPL_RT_MD_FILENAME, -1, 0);
sprintf(stdoutFileNoFmt, "%s", outputfn);
}
else {
sprintf(stdoutFile, "%s.%s.out", argv[0], "%j");
sprintf(stdoutFileNoFmt, "%s.%s.out", argv[0], "$SLURM_JOB_ID");
char* format="%s.%s.out";
int stdoutFileLen = strlen(format) + strlen(argv[0])+ strlen("%j");
stdoutFile=(char *)chpl_mem_allocMany(stdoutFileLen, sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
sprintf(stdoutFile, format, argv[0], "%j");
char* tempArg = "$SLURM_JOB_ID";
int stdoutFileNoFmtLen = strlen(format) + strlen(argv[0]) + strlen(tempArg);
stdoutFileNoFmt=(char *)chpl_mem_allocMany(stdoutFileNoFmtLen,
sizeof(char), CHPL_RT_MD_FILENAME, -1, 0);
sprintf(stdoutFileNoFmt, format, argv[0], tempArg);
}

// We have slurm use the real output file to capture slurm errors/timeouts
// We only redirect the program output to the tmp file
fprintf(slurmFile, "#SBATCH --output=%s\n", stdoutFile);
Expand All @@ -324,7 +338,13 @@ static char* chpl_launch_create_command(int argc, char* argv[],
// If we're buffering the output, set the temp output file name.
// It's always <tmpDir>/binaryName.<jobID>.out.
if (bufferStdout != NULL) {
sprintf(tmpStdoutFileNoFmt, "%s/%s.%s.out", tmpDir, argv[0], "$SLURM_JOB_ID");
char* format = "%s/%s.%s.out";
char* tempArg = "$SLURM_JOB_ID";
int tmpStdoutFileNoFmtLen = strlen(format) + strlen(tmpDir) +
strlen(argv[0]) + strlen(tempArg);
tmpStdoutFileNoFmt = (char *)chpl_mem_allocMany(tmpStdoutFileNoFmtLen,
sizeof(char), CHPL_RT_MD_FILENAME, -1, 0);
sprintf(tmpStdoutFileNoFmt, format, tmpDir, argv[0], tempArg);
Copy link
Member

Choose a reason for hiding this comment

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

In cases like this where you've computed and stored the buffer length, wouldn't it be safer / more future-proof to use snprintf() rather than sprintf() (to avoid buffer overrun in case someone changes one part of the malloc/snprintf pattern in the future and doesn't change the other part to match?). Essentially, I'm curious whether there was a rationale for using sprintf() in some cases and snprintf() in others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradcray

I'm curious whether there was a rationale for using sprintf() in some cases and snprintf() in others.

I kept the usage of snprintf() and sprintf() the same as in the code before changes (this PR). But, now since we have the lengths of the char arrays with us, it would be better (safer) to convert sprintf() to snprintf(), if we have the lengths of the char array already stored. This would also be more robust to changes in future, rather than sprintf(). I will make these changes.
Thanks,
R. Chinmay

Copy link
Member

Choose a reason for hiding this comment

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

Your rationale makes sense, but if you're willing to make that change while here, I think that'd be great, thank you!

}

// add the srun command and the (possibly wrapped) binary name.
Expand All @@ -348,6 +368,9 @@ static char* chpl_launch_create_command(int argc, char* argv[],
if (bufferStdout != NULL) {
fprintf(slurmFile, "cat %s >> %s\n", tmpStdoutFileNoFmt, stdoutFileNoFmt);
fprintf(slurmFile, "rm %s &> /dev/null\n", tmpStdoutFileNoFmt);
//tmpStdoutFileNoFmt is only allocated memory if bufferStdout!=NULL
//Hence free up inside this condition.
chpl_mem_free(tmpStdoutFileNoFmt);
}

// close the batch file and change permissions
Expand All @@ -360,7 +383,11 @@ static char* chpl_launch_create_command(int argc, char* argv[],

// the baseCommand is what will call the batch file
// that was just created
sprintf(baseCommand, "sbatch %s\n", slurmFilename);
char* format = "sbatch %s\n";
int baseCommandLen = strlen(slurmFilename) + strlen(format);
baseCommand=(char *)chpl_mem_allocMany(baseCommandLen, sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
sprintf(baseCommand, format, slurmFilename);
}
// else we're running an interactive job
else {
Expand Down Expand Up @@ -431,15 +458,22 @@ static char* chpl_launch_create_command(int argc, char* argv[],
for (i=1; i<argc; i++) {
len += sprintf(iCom+len, "%s ", argv[i]);
}

char* format = "srun %s ";
int baseCommandLen = strlen(format) + len;
baseCommand=(char *)chpl_mem_allocMany(baseCommandLen, sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
// launch the job using srun
sprintf(baseCommand, "srun %s ", iCom);
sprintf(baseCommand, format, iCom);
}

// copy baseCommand into command and return it
size = strlen(baseCommand) + 1;
command = chpl_mem_allocMany(size, sizeof(char), CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
sprintf(command, "%s", baseCommand);
//free dynamically allocated memory
chpl_mem_free(baseCommand);
chpl_mem_free(stdoutFile);
chpl_mem_free(stdoutFileNoFmt);
if (strlen(command)+1 > size) {
chpl_internal_error("buffer overflow");
}
Expand All @@ -460,7 +494,9 @@ static void chpl_launch_cleanup(void) {
// remove sbatch file unless it was explicitly generated by the user
if (getenv("CHPL_LAUNCHER_USE_SBATCH") != NULL && !generate_sbatch_script) {
if (unlink(slurmFilename)) {
char msg[FILENAME_MAX + 128];
char* msg=(char *)chpl_mem_allocMany((strlen(slurmFilename) +
strlen(strerror(errno)) + 36),
sizeof(char), CHPL_RT_MD_FILENAME, -1, 0);
snprintf(msg, sizeof(msg), "Error removing temporary file '%s': %s",
slurmFilename, strerror(errno));
chpl_warning(msg, 0, 0);
Expand Down Expand Up @@ -495,6 +531,7 @@ int chpl_launch(int argc, char* argv[], int32_t numLocales) {

chpl_launch_cleanup();
}
chpl_mem_free(slurmFilename);
return retcode;
}

Expand Down