Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix crash in ecp_nistz256_point_add_affine() #24192

Closed
wants to merge 1 commit into from
Closed

fix crash in ecp_nistz256_point_add_affine() #24192

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion crypto/perlasm/x86_64-xlate.pl
View file
Open in desktop
Expand Up @@ -909,29 +909,55 @@
# to deal with nasm/masm assembly.
#
$self->{value} =~ s/(.+)\s+align\s*=.*$/$1/;
$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");
}
Comment on lines +912 to +918
Copy link
Member

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.

Copy link
Contributor Author

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 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.

Copy link
Member

@t8m t8m Apr 23, 2024 •
edited

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

#
# $$line may still contains align= option. We do care
# about section type here.
#
$current_segment = $$line;
$current_segment =~ s/([^\s]+).*$/$1/;
push(@segment_stack, $current_segment);
if (!$elf && $current_segment eq ".rodata") {
if ($flavour eq "macosx") { $self->{value} = ".section\t__DATA,__const"; }
elsif ($flavour eq "mingw64") { $self->{value} = ".section\t.rodata"; }
}
if (!$elf && $current_segment eq ".init") {
if ($flavour eq "macosx") { $self->{value} = ".mod_init_func"; }
elsif ($flavour eq "mingw64") { $self->{value} = ".section\t.ctors"; }
}
} elsif ($dir =~ /\.(text|data)/) {
$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");
}
t8m marked this conversation as resolved.
Show resolved Hide resolved
$current_segment=".$1";
push(@segment_stack, $current_segment);
} 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
# just peek at the top of the stack here
$current_segment = @segment_stack[0];
if (not $current_segment) {
# if no previous segment was defined assume .text so
# the code does not accidentally land in .data section.
$current_segment = ".text";
push(@segment_stack, $current_segment);
}
$self->{value} = $current_segment if ($flavour eq "mingw64");
}
$$line = "";
return $self;
Expand Down