fix crash in ecp_nistz256_point_add_affine()

[Sashan]

The .rodata section with precomputed constant ecp_nistz256_precomputed needs to be terminated by .previous. The lack of .previous makes mingw compiler to put code into read only section. The exception is raised as soon as CPU attempts to execute the code from read only section.

Fixes #24184

[t8m]

Hmm... the dos2unix hack is really ugly. Is it a problem with the perl installation on mingw that it outputs CRLF? Could it be somehow mitigated within the perl execution?

I'd suggest splitting this to 2 PRs - one with tests: deferred that fixes the reported problem and another one that would try to add CI for cygwin and mingw builds as these are something we did not have before and I assume it will take quite some effort to make them right.

[Sashan]

I've removed changes to workflow. I agree dos2unix hack is ugly, but it gets tests going. Unfortunately there are other hurdles in cygwin environment which prevent tests from successful completion. hands-on access to cygwin environment is needed to sort it out.

another option would be to let cygwin build library and then use MSVC (windows) to build tests and link with library produced by cygwin. this is just a thought..,

This does not make much sense to me here - it cannot happen as there will be either text or data from the regexp match. Maybe the ordering is wrong?

yes, good catch the code should read as follows:

 934                 } elsif ($dir =~ /\.(text|data)/) {
 935                     $current_segment = pop(@segment_stack);
 936                     if (not $current_segment) {
 937                         # if no previous section is defined, then assume .text
 938                         # so code does not land in .data section by accident.
 939                         # this deals with inconsistency of perl-assembly files.
 940                         push(@segment_stack, ".text");
 941                     }
 942                     $current_segment=".$1";
 943                     push(@segment_stack, $current_segment);
 944                 } elsif ($dir =~ /\.hidden/) {

thanks for catching this.

will update pull request shortly

[t8m]

Why do you pop here? It does not make sense to me. You should just pop on .previous, shouldn't you? And preinitialize @segment_stack as (".text"). Then, if the assembler .pl files are correctly written, you should never get to empty stack.

[Sashan]

Keep in mind the .section directive serves two purposes:

• it closes the preceding section (if there is any)
• it starts a new section
  Hence we do the pop() on .section too. Consider the .s file is structured as follows:

.section data
   .byte ....
.section text
    mov %ecx, ...
.section data
    ...
.section text
    ...

The sections are not nested. It's a list of sections: [data, text, data, text]. If assembler flavour supports .previous directive one can define the same structure like that:

.section data
    .byte ...
.section text
    mov %ecx, ...
.section data
    ...
.previous
    ...

The difference is that for .previous directive we use the stack to remember which section is the previus one so we can emit the correct section header. In this particular example the .previous becomes .section text. If we are dealing with .section we just pop() the element and forget it because the .section comes with an explicit definition.

[t8m]

Hmm, right, but then we do not actually need a stack but just a prev_section scalar variable initialized to ".text" at the beginning. And .previous after .previous without any other section in between is either a syntax error or a no-op.

[Sashan]

yes, in case of mingw we don't need it. We need stack for MASM flavour (Microsoft assembler) where .previous directive expends to two directives:

.data ENDS
.text SEGMENT....

However I'd like to keep kind of consistent. the stack is overly complicated for anything except masm. on the other hand it's generic enough so it can help us with any flavour. So I think it makes kind of sense to use it for mingw64 flavour instead of introducing yet another logic.

[t8m]

Hmm... I believe you could implement it without a stack even for MASM with just two variables but I won't fight for it.

[t8m]

Instead of popping I'd use $segment_stack[-1] and then you do not need to push again.

[Sashan]

We can go for this code in to handle .previous section:

 950                 } elsif ($dir =~ /\.previous/) {
 951                     pop(@segment_stack); #pop ourselves
 952                     # just peek at the top of the stack here
 953                     $current_segment = @segment_stack[0];
 954                     if (not $current_segment) {
 955                         # if no previous segment was defined assume .text so
 956                         # the code does not accidentally land in .data section.
 957                         $current_segment = ".text"
 958                         push(@segment_stack, $current_segment);
 959                     }
 960                     $self->{value} = $current_segment if ($flavour eq "mingw64");
 961                 }

Will update the pull request shortly.

[Andarwinux]

Hi. Friendly ping. What is the current status of this PR? We hope to merge this soon so that we can continue to use openssl master without having to revert any commits or apply any patches.

[t8m]

ping for second review