Add a GithubCI job to test oqs-provider against memory leaks.

[thb-sb]

This commit introduces a new CircleCI job called check-ASan-tests that compiles OpenSSL 3, liboqs and oqs-provider with ASan (-fsanitize=address). Then, it runs the CTest suite.

If any memory leak occurs somewhere and is triggered in one of the various unit tests, then this CircleCI job fails and displays the ASan trace.

This job should fail until #245 is merged.

[baentsch]

This job should fail until #245 is merged.

And it duly does :)

Can I ask whether there was a reason this job was added in CCI instead of github CI? Please see (the newly added #248).

[baentsch]

LGTM. Thanks for this "long-term" memory consistency protection for the project! Please feel free to merge if you'd like to keep this on CCI for some reason and for the time being. Otherwise, a switch to github CI would be nice.

[thb-sb]

Can I ask whether there was a reason this job was added in CCI instead of github CI? Please see (the newly added #248).

There is no reason to that, it's just that I'm used to CircleCI, but let me try to move it to GitHub actions instead.!

[thb-sb]

@baentsch I moved the ASan test to GitHub actions in e65ebe2.

Since it's the first time I use it, I'd prefer you to check my code before merging, @baentsch

[baentsch]

Since it's the first time I use it, I'd prefer you to check my code before merging, @baentsch

OK -- sorry I noticed this update here only now. The code looks good (and failure is as expected :) -- but is keeping it in a separate job the best approach? Reason for asking: The way it is right now it's not really represented "at a glance" (in the README.md's CI status buttons). Given it's really a Linux test, would it be (more) sensible to add it within the existing Linux test (to see a failure suitably highlighted?). Also it's not really telling us too much about Windows or MacOS memory leaks, does it? It's unlikely there should be (different) leaks on those platforms but we don't really know without running an "asan-equivalent" there, right?

[thb-sb]

but is keeping it in a separate job the best approach? Reason for asking: The way it is right now it's not really represented "at a glance" (in the README.md's CI status buttons).

Ho, I haven't thought about it.

Given it's really a Linux test, would it be (more) sensible to add it within the existing Linux test (to see a failure suitably highlighted?)

Should I put it in the .github/workflows/linux.yml file then?
I should read some documentation about GitHub action workflows, I really don't know what the CI status buttons display actually…

Also it's not really telling us too much about Windows or MacOS memory leaks, does it? It's unlikely there should be (different) leaks on those platforms but we don't really know without running an "asan-equivalent" there, right?

I know that we can compile everything with ASan on MacOS, but I'm not sure about Windows.
I can add a ASan test in the .github/workflows/macos.yml file too

[baentsch]

Should I put it in the .github/workflows/linux.yml file then?

That would (have) be(en) my preference, Yes. Before you do it, please read on.

I really don't know what the CI status buttons display actually…

They simply show whether the latest run (on the given branch --"main" in our case) was successful or not. So any test failure would be flagged (and make us aware of something wrong).

I know that we can compile everything with ASan on MacOS, but I'm not sure about Windows.
I can add a ASan test in the .github/workflows/macos.yml file too

I just very nearly stated "If we can do MacOS too, then Yes, please also add it there." But I'm reconsidering/wondering: Is Asan really finding platform-specific problems or any issues with the code (happening on every platform)? If the latter were the case, I'd revisit my statements from above and keep things as you did them now (and only ask you to change the job title to "Safety Checks" and add that job's CI button to the README): What's your take, @thb-sb ?

[thb-sb]

They simply show whether the latest run (on the given branch --"main" in our case) was successful or not. So any test failure would be flagged (and make us aware of something wrong).

Okay, got it! Thank you.

But I'm reconsidering/wondering: Is Asan really finding platform-specific problems or any issues with the code (happening on every platform)?

It may happen that some bits of code are platform-specific, but usually it's all about optimization using platform-specific hw instructions etc, and it would be very unlikely that those bits have something to do with allocating / de-allocating things. Do you agree on that?

If the latter were the case, I'd revisit my statements from above and keep things as you did them now (and only ask you to change the job title to "Safety Checks" and add that job's CI button to the README): What's your take, @thb-sb ?

Since I consider that we're not in that case, I'll move the ASan test in the linux.yml workflows file, and I'll rename it to "Safety Checks".

[baentsch]

Runs in parallel to the other Linux jobs, i.e., doesn't lie on the critical path or add CI time: LGTM. Thanks again. OK to merge? Will you want to rebase #245 afterwards?

[thb-sb]

OK to merge? Will you want to rebase #245 afterwards?

Yes, OK to merge and then I'll rebase 245 on top of main. :)

Thank you for your comments.!