Replacing FILENAME_MAX in runtime-directory

[rchinmay]

Replaces char arrays in runtime directory, which used FILENAME_MAX macro for its size, to dynamic memory allocation using malloc and free functions.

[rchinmay]

Resolves #8757 for runtime directory.
@bradcray @daviditen Please review.
Thanks,
R. Chinmay

[bradcray]

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);

[bradcray]

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.

[rchinmay]

@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

[bradcray]

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

[bradcray]

I think this is getting close, thanks @rchinmay (and apologies again for my slow response... you probably regret getting me as the lead reviewer).

I wanted to tag @ronawho here to see if he had thoughts about what I should do to test this before merging (I suspect there's no easy answer given that it's distinct launchers; but maybe I could test them all locally using asan just to make sure they "failed to launch" and didn't have memory errors, and then left the stress testing for the nightlies?)

Also tagging @cassella since he was the impetus behind the original issue #8757.

[ronawho]

I wanted to tag @ronawho here to see if he had thoughts about what I should do to test this before merging (I suspect there's no easy answer given that it's distinct launchers; but maybe I could test them all locally using asan just to make sure they "failed to launch" and didn't have memory errors, and then left the stress testing for the nightlies?)

That's an interesting idea, but it'll probably fail in unhelpful ways today since some launchers call things like sinfo or cnselect to build the launch commands so you'll probably get early failures. I wonder if we could add a --dry-run option or something that would fake values for calls and then print out what command the launcher would use. Kinda like -v, but don't actually launch and don't require launcher utilities like cnselect/sinfo

For this PR, I think just ping me when you're ready for testing and I'll spot check a few launchers.

[bradcray]

some launchers call things like sinfo or cnselect to build the launch commands

Oh, good point.

For this PR, I think just ping me when you're ready for testing and I'll spot check a few launchers.

That'd be very nice, thanks. I'd also be curious whether you'd prefer to hold off on merging this PR until after the release to live with it for awhile, or whether you think between your spot-checking and nightly testing we'd probably be safe enough.

[cassella]

Hrm. In launch-pbs-aprun.c, I'm seeing the function genQsubOptions() sprintf()s into the global expectFilename:

static char* genQsubOptions(char* genFilename, char* projectString, qsubVersion qsub, 
                            int32_t numLocales, int32_t numCoresPerLocale) {
...
  char *qsubFilename = expectFilename;
...

  if (generate_qsub_script) {
    pid_t mypid = debug ? 0 : getpid();
    sprintf(qsubFilename, "qsub.%s-%d", genFilename, (int) mypid);

which may be a longer string.

This is pretty gross. If it were up to me, I'd make expectFilename local to chpl_launch_create_argv(), and give genQsubOptions() its own char * to manage.

(It's not straightforward since genQsubOptions() uses the buffer and returns it in one case, and returns it unmodified in the other. I think this is all just so the file didn't need to have two static buffers in it. If so, I think it would be cleaner if they didn't try to share like that. I don't know how much of a mess it'll be to straighten that out.)

[bradcray]

@cassella : Just to make sure I'm following, your last comment is unrelated to this specific effort (retiring the use of FILENAME_MAX), is that right? (I understand that it's thematically related in that it relates to how launcher string buffers are allocated and populated).

Assuming so, I propose we consider that a pre-existing condition and not worry about it here. If something about this PR makes the existing code less attractive than it was, then I'd worry about it here.

[cassella]

@bradcray No, it is a new problem with this PR.

Currently, genQsubOptions() sprintf()s into the expectFilename buffer. Its pattern is small and the buffer is FILENAME_MAX, so that's probably fine (or at least, it has been thus far).

With the PR, it's still sprintf()ing into the same buffer, but now the buffer is sized precisely according to the batch filename and pid. Depending on what genQsubOptions()'s genFilename is, it may be sprintfing something larger than the buffer.

The "if it were up to me" part is just what I'd do about it, versus (I guess?) sizing the single array to the max of the two uses' lengths.

[ronawho]

That'd be very nice, thanks. I'd also be curious whether you'd prefer to hold off on merging this PR until after the release to live with it for awhile, or whether you think between your spot-checking and nightly testing we'd probably be safe enough.

At this point I'd probably hold off until after the release. (I was hoping to get to this last week, but other release tasks have been taking up all my time)

[bradcray]

@rchinmay : Now that we're through with the release, I wanted to ping you to see whether you're still interested in pursuing this effort? I think there were a few minor clean-up items in my last review that it looked like you'd intended to address; and then there's the more fundamental issue that @cassella brings up above.

[rchinmay]

@bradcray, Thanks for informing on the release. I saw @cassella 's comment and I am currently working on it to solve the problem. Along with that, I will also make the clean-ups suggested by you.
Thanks,
R. Chinmay.

[bradcray]

Great, thanks for the update!

[bradcray]

@rchinmay : I'm looking through PRs that I'm associated with today and wanted to check in on this one. Are you still working on it? Thanks.

[bradcray]

I'm going to park this until someone has a chance to get back to it.

[riftEmber]

Picking this work back up in #26381, which incorporates the commits from this PR branch.