RFR: 8320219: Actually resolve issues with goto labels in sspi [v3]

[Thomas Stuefe]

On Thu, 16 Nov 2023 04:40:53 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> I regret not actually addressing the issues with the goto labels in https://github.com/openjdk/jdk/pull/15996, where initialization of locals in sspi were jumped over by gotos to a certain label. I changed the initializations into split declarations and assignments in https://github.com/openjdk/jdk/pull/15996, but this is simply a hack and does not address the real issue of gotos jumping over locals. I've as such fixed the issues with them properly this time, by simply deleting the labels and duplicating the code where they're used. As mentioned, this unfortunately does increase duplicate code, but is the cleanest solution I could come up with for the labels
>
> Julian Waters has updated the pull request incrementally with one additional commit since the last revision:
> 
>   NULL to nullptr in sspi.cpp

> > > > Can you please describe the problem you are trying to solve, and why you think it is worth solving. You describe the thought process you went through while doing the patch and possibly some other, older patch. As it is, reading the PR text or the issue description is confusing and requires reviewers to dig through comment threads on older issues to parse it.
> > > 
> > > 
> > > I've changed to description for both to (hopefully) be more descriptive and clear. Sorry for the hassle, I'm not particularly good with words
> > 
> > 
> > Somewhat better, but not by much, sorry :-)
> > This patch is work to review, since it touches security-relevant stuff and because mentally verifying that all the code paths you change are equivalent to what happened before is real work. It is probably more work than doing the patch.
> > Hence my question, what is the motivation, is there a bug that needs fixing? Or is it just that you dislike these gotos? Is it just to get Windows to build with GCC? Sorry, I did not follow your work, so I don't have a context.
> 
> This is a cleanup patch that follows the older one that enabled security to compile with permissive-, the problem with that one is that simply changing the locals to a split declaration and assignment is a bit of an ugly hack, since the locals then have undefined values when allocated. gcc already compiles fine with the old patch, this new one is not needed for either compiler (MSVC or gcc) to compile without errors. I felt like I had the responsibility to clean up what is essentially a hack just to get it to compile with permissive- on MSVC. This is also partially inspired by Phil's rejection of the old patch in #15096 :/
> 
> I don't have anything against gotos, nor do I dislike them :)

Thank you for your explanation! I'll relegate to Phil for the actual review then; I was just curious about the patch since it touched security-related code.

Cheers, Thomas

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16682#issuecomment-1816661606