[#2156] Replace variable length arrays with alloca() (main)

[SwooshyCueb]

Addresses #2156 (for real this time)

[korydraughn]

The changes look fine, but I'm not sold on the use of alloca yet.

I don't have any experience with it, but reading about it indicates we should probably rethink using it.

The main thing is that we're replacing VLAs with alloca, which seems incorrect.

Thoughts?

[SwooshyCueb]

I'm not sure I understand the problem. alloca() allocates on the stack, just as a VLA does.

[korydraughn]

Right. I'm concerned about stack overflow and undefined behavior.

From alloca's man page (https://man7.org/linux/man-pages/man3/alloca.3.html):

The alloca() function returns a pointer to the beginning of the
allocated space.  If the allocation causes stack overflow,
program behavior is undefined.

The amount of stack space needed is unknown for the some/all of the VLAs.

Here's another link on it: https://stackoverflow.com/a/1018865/5818611

Another thing that comes to mind is if something weird happens as a result of using alloca, will we be able to track down the cause?

[korydraughn]

The NOTES section of the man page makes me slightly nervous too.

[SwooshyCueb]

Looking at the comments in the stackoverflow answer you linked, it appears that VLAs have the exact same problems as alloca() in regards to stack overflows.

What do you find concerning in the NOTES section?

[korydraughn]

That section states its use is discouraged. I take that as the dev must be sure that function is the correct choice for the task and it doesn't leave room for stack overflow.

[SwooshyCueb]

If my understanding is correct, given our usage, if a stack overflow would occur with alloca(), it would occur with VLAs as well. Therefore, if we have a potential stack overflow issue, we had it prior to this PR. The case can certainly be made that we should not be allocating space on the stack at runtime like this in the first place, but that's outside the scope of this PR.

[korydraughn]

That's a fair point.

Please run the test suite to confirm everything is still good.

[SwooshyCueb]

All tests fail for tip of main, so they fail here too.

[korydraughn]

That's unexpected.

Was anything reported that indicates the reason for the failures?
Does it fail at the same place / in the same way on every run?

[SwooshyCueb]

So I forgot that the testing environment will report that all tests fail for plugin tests regardless of how many actually fail, and it turns out that on tip of main, only most tests fail. I think it may have something to do with irods/irods#7502. I'll open an issue about that.

For this PR, all tests did actually fail, and it appeared to be due to a signature mismatch. I figured out that this was due to a difference in how sizeof() works with alloca() vs VLAs, so I've replaced the relevant instances of sizeof() (see the new [SQUASH] commit), and now the test failures match tip of main.

I don't like clang-format's suggestion for the canonicalRequestLen definition, so I'm ignoring it.

[korydraughn]

and now the test failures match tip of main.

Are the failing tests you're referring to part of the irods/irods test suite or this repo's test suite?

I don't like clang-format's suggestion for the canonicalRequestLen definition, so I'm ignoring it.

Agreed. Wrap it in // clang-format off/on so future edits don't yell at us about it.

[SwooshyCueb]

Are the failing tests you're referring to part of the irods/irods test suite or this repo's test suite?

They're all from this repo

[korydraughn]

Are the tests that fail expected? I don't remember that being the case.

[SwooshyCueb]

I don't think so. I think they're failing because they weren't updated to expect the deprecation message introduced in irods/irods#7502. I'm still looking into it. There may be other tests failing for different reasons.

[korydraughn]

Got it. Will come back once you've learned more.

[SwooshyCueb]

Changing the tests to expect the deprecation message fixed some of them, but another one, test_irods_resource_plugin_s3_minio.Test_Compound_With_S3_Resource.test_irepl_multithreaded fails for what seems to be an unrelated reason, and leaves the server in a state that causes every subsequent test to fail. This test is inherited from ResourceBase in the irods repo. I'll open another issue for this.

[SwooshyCueb]

I've opened #2184 to cover the remaining test failures. The cause of the failure wasn't evident to me so I'm going to leave that work to someone else so I can get back to working on buildsystem and packaging stuff.

[SwooshyCueb]

If there are no objections, I'm going to # this and the 4-3-stable PR

[alanking]

I think I'm okay with the changes since alloca presents nothing more dangerous than VLAs. I just saw one thing in the tests.

Deferring to @korydraughn, though.

[korydraughn]

Let's hold on pounding. We need to determine why the tests are failing. Once that's handled we can proceed.

[SwooshyCueb]

We merged the test and include fixes in a separate PR (#2189, #2190) so they could make it in for 4.3.2. They've been removed from this PR (and the 4-3-stable PR).

[trel]

Going to wait until after 4.3.2 to merge this PR.

[SwooshyCueb]

I've confirmed that tests pass now. If there are no objections, I will # this PR and the 4-3-stable PR.

[korydraughn]

Pound it.

[SwooshyCueb]

#'d