New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix crash in ecp_nistz256_point_add_affine() #24192
Conversation
14cdaea
to
949e322
Compare
adb09bc
to
3f0be53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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..,
yes, good catch the code should read as follows:
thanks for catching this. will update pull request shortly |
$current_segment = pop(@segment_stack); | ||
if (not $current_segment) { | ||
# if no previous section is defined, then assume .text | ||
# so code does not land in .data section by accident. | ||
# this deals with inconsistency of perl-assembly files. | ||
push(@segment_stack, ".text"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thepop()
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I believe you could implement it without a stack even for MASM with just two variables but I won't fight for it.
crypto/perlasm/x86_64-xlate.pl
Outdated
} elsif ($dir =~ /\.hidden/) { | ||
if ($flavour eq "macosx") { $self->{value} = ".private_extern\t$prefix$$line"; } | ||
elsif ($flavour eq "mingw64") { $self->{value} = ""; } | ||
} elsif ($dir =~ /\.comm/) { | ||
$self->{value} = "$dir\t$prefix$$line"; | ||
$self->{value} =~ s|,([0-9]+),([0-9]+)$|",$1,".log($2)/log(2)|e if ($flavour eq "macosx"); | ||
} elsif ($dir =~ /\.previous/) { | ||
$self->{value} = "" if ($flavour eq "mingw64"); | ||
pop(@segment_stack); #pop ourselves | ||
$current_segment = pop(@segment_stack); #pop previous section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of popping I'd use $segment_stack[-1]
and then you do not need to push again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The .rodata section with precomputed constant `ecp_nistz256_precomputed` needs to be terminated by .text, because the ecp_nistz256_precomputed' happens to be the first section in the file. The lack of .text makes code to arrive into the same .rodata section where ecp_nistz256_precomputed is found. The exception is raised as soon as CPU attempts to execute the code from read only section. Fixes openssl#24184
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. |
ping for second review |
Is this PR dead? |
This pull request is ready to merge |
Merged to the master branch. Thank you. |
The .rodata section with precomputed constant `ecp_nistz256_precomputed` needs to be terminated by .text, because the ecp_nistz256_precomputed' happens to be the first section in the file. The lack of .text makes code to arrive into the same .rodata section where ecp_nistz256_precomputed is found. The exception is raised as soon as CPU attempts to execute the code from read only section. Fixes #24184 Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24192)
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