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

Modify jdk.crypto.ec libsunec mpi.c to avoid writes to unallocated mem #726

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Modify jdk.crypto.ec libsunec mpi.c to avoid writes to unallocated mem #726

merged 1 commit into from
Nov 9, 2023

Conversation

pshipton
Copy link
Member

@pshipton pshipton commented Nov 7, 2023 •
edited

See https://openj9-jenkins.osuosl.org/job/Build_JDK11_s390x_linux_Personal/767/ for the gcc 11.2 compile error on the original code.

18:08:54  In function 'mp_zero',
18:08:54      inlined from 'mp_zero' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:316:8,
18:08:54      inlined from 'mp_set_int' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:353:3,
18:08:54      inlined from 'mp_cmp_int' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:1735:26:
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:321:3: error: 'tmp.dp' may be used uninitialized [-Werror=maybe-uninitialized]
18:08:54    321 |   s_mp_setz(DIGITS(mp), ALLOC(mp));
18:08:54        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c: In function 'mp_cmp_int':
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:1730:11: note: 'tmp' declared here
18:08:54   1730 |   mp_int  tmp;
18:08:54        |           ^~~
18:08:54  In function 'mp_zero',
18:08:54      inlined from 'mp_zero' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:316:8,
18:08:54      inlined from 'mp_set_int' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:353:3,
18:08:54      inlined from 'mp_cmp_int' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:1735:26:
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:321:3: error: 'tmp.alloc' may be used uninitialized [-Werror=maybe-uninitialized]
18:08:54    321 |   s_mp_setz(DIGITS(mp), ALLOC(mp));
18:08:54        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c: In function 'mp_cmp_int':
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:1730:11: note: 'tmp' declared here
18:08:54   1730 |   mp_int  tmp;
18:08:54        |           ^~~
18:08:54  In function 'mp_zero',
18:08:54      inlined from 'mp_zero' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:316:8,
18:08:54      inlined from 'mp_set_int' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:353:3,
18:08:54      inlined from 'mp_cmp_int' at /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:1735:26:
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:321:3: error: 'tmp.alloc' may be used uninitialized [-Werror=maybe-uninitialized]
18:08:54    321 |   s_mp_setz(DIGITS(mp), ALLOC(mp));
18:08:54        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c: In function 'mp_cmp_int':
18:08:54  /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:1730:11: note: 'tmp' declared here
18:08:54   1730 |   mp_int  tmp;
18:08:54        |           ^~~
18:08:55  cc1: all warnings being treated as errors
18:08:55  Lib-jdk.crypto.ec.gmk:41: recipe for target '/home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/build/linux-s390x-normal-server-release/support/native/jdk.crypto.ec/libsunec/mpi.o' failed

Tested via
https://openj9-jenkins.osuosl.org/job/Pipeline-Build-Test-Personal/448/
https://openj9-jenkins.osuosl.org/job/Pipeline-Build-Test-Personal/452/

@pshipton
Copy link
Member Author

pshipton commented Nov 7, 2023 •
edited

Not sure my original fix is the best solution. It seems safe with the current usage of mp_cmp_int() but perhaps asserting if there is an error is a better way to go.

@pshipton
Copy link
Member Author

pshipton commented Nov 7, 2023

I've updated to implement it without allocation.

keithc-ca
keithc-ca reviewed Nov 8, 2023
View reviewed changes
src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c Outdated
int out;

mp_sign signz = z < 0 ? ZNEG : ZPOS;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the trailing whitespace here.
This file should have an IBM copyright notice.

Copy link
Member Author

@pshipton pshipton Nov 8, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and running new testing via https://openj9-jenkins.osuosl.org/job/Pipeline-Build-Test-Personal/451/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

[2023-11-08T22:05:46.059Z] /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c: In function 'mp_cmp_int':
[2023-11-08T22:05:46.059Z] /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal/src/jdk.crypto.ec/share/native/libsunec/impl/mpi.c:1736:27: error: 'ZNEG' undeclared (first use in this function); did you mean 'NEG'?

ZNEG should be NEG (see e.g. line 376); I suppose NEG means "negative" and ZPOS means "zero or positive".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://openj9-jenkins.osuosl.org/job/Pipeline-Build-Test-Personal/452/

@keithc-ca
Copy link
Member

This is not just to please the compiler, it fixes a potential real problem (misbehavior on allocation failure): perhaps the title and description should be updated.

@pshipton pshipton changed the title Modify jdk.crypto.ec libsunec/impl/mpi.c to compile with gcc11.2 Modify jdk.crypto.ec libsunec mpi.c to avoid writes to unallocated mem Nov 8, 2023
@pshipton
Modify jdk.crypto.ec libsunec mpi.c to avoid writes to unallocated mem
e9695f5 
Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>
@pshipton pshipton mentioned this pull request Nov 9, 2023
Merged
@keithc-ca keithc-ca merged commit 5324599 into ibmruntimes:openj9 Nov 9, 2023
2 checks passed
@pshipton pshipton mentioned this pull request Nov 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pshipton @keithc-ca