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

util/ck_errf.pl: add detection of unknown libcrypto and libssl libs #6455

Closed
wants to merge 2 commits into from
Closed

util/ck_errf.pl: add detection of unknown libcrypto and libssl libs #6455

wants to merge 2 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Jun 11, 2018

The list of known libs are readily available in crypto/err/openssl.ec,
so lets use it to figure out if all error function codes belong to
known libs.

@levitte levitte added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Jun 11, 2018
@levitte
Copy link
Member Author

levitte commented Jun 11, 2018

This is an alternative for #6452

@levitte levitte mentioned this pull request Jun 11, 2018
Closed
richsalz
richsalz approved these changes Jun 11, 2018
View reviewed changes
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

Assuming you add the comment: approved.

util/ck_errf.pl Outdated
@@ -19,6 +19,16 @@
my $err_strict = 0;
my $bad = 0;

my $config = "crypto/err/openssl.ec";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like a comment here explaining what's being done and why.

@levitte
util/ck_errf.pl: add detection of unknown libcrypto and libssl libs
b1abcdf 
The list of known libs are readily available in crypto/err/openssl.ec,
so lets use it to figure out if all error function codes belong to
known libs.
@levitte
fixup! util/ck_errf.pl: add detection of unknown libcrypto and libssl…
cd936da 
… libs
@levitte levitte force-pushed the make-ck_errf-report-unknown-libs branch from 2bda3d6 to cd936da Compare June 11, 2018 14:58
@levitte
Copy link
Member Author

levitte commented Jun 11, 2018

Heh, you know that if you approve with a change request like that, I will ask you to reconfirm...

Comment looking good?

@richsalz
Copy link
Contributor

comment is nice. reconfirm.

@levitte
Copy link
Member Author

levitte commented Jun 11, 2018

Merged.

a21180b util/ck_errf.pl: add detection of unknown libcrypto and libssl libs

@levitte levitte closed this Jun 11, 2018
levitte added a commit that referenced this pull request Jun 11, 2018
@levitte
util/ck_errf.pl: add detection of unknown libcrypto and libssl libs
a21180b 
The list of known libs are readily available in crypto/err/openssl.ec,
so lets use it to figure out if all error function codes belong to
known libs.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #6455)
@DDvO
Copy link
Contributor

DDvO commented Jun 11, 2018

I like the solution.

Just two minor drawbacks (compared with the initial solution, which had other drawbacks):

  • The error message just says 'unknown' - would be nice if it also said: 'in crypto/err/openssl.ec'.
  • In case there is no error in at the LIBerr() invocations but in oenssl.ec, one error line would be sufficient.
    On the other hand I see the advantage of the new solution that it can also spot issues with the individual invocations.

Take for instance the ouput of perl util/ck_errf.pl -strict */*.c */*/*.c, which currently is:

engines/e_afalg.c:165:AFALG unknown
engines/e_afalg.c:189:AFALG unknown
engines/e_afalg.c:207:AFALG unknown
engines/e_afalg.c:356:AFALG unknown
engines/e_afalg.c:380:AFALG unknown
engines/e_afalg.c:387:AFALG unknown
engines/e_afalg.c:394:AFALG unknown
engines/e_afalg.c:722:AFALG unknown
engines/e_afalg.c:733:AFALG unknown
engines/e_afalg.c:739:AFALG unknown
engines/e_afalg.c:775:AFALG unknown
engines/e_afalg.c:792:AFALG unknown
engines/e_afalg.c:800:AFALG unknown
engines/e_capi.c:290:CAPI unknown
engines/e_capi.c:296:CAPI unknown
engines/e_capi.c:323:CAPI unknown
engines/e_capi.c:350:CAPI unknown
engines/e_capi.c:370:CAPI unknown
engines/e_capi.c:386:CAPI unknown
engines/e_capi.c:489:CAPI unknown
engines/e_capi.c:637:CAPI unknown
engines/e_capi.c:648:CAPI unknown
engines/e_capi.c:655:CAPI unknown
engines/e_capi.c:667:CAPI unknown
engines/e_capi.c:713:CAPI unknown
engines/e_capi.c:760:CAPI unknown
engines/e_capi.c:778:CAPI unknown
engines/e_capi.c:792:CAPI unknown
engines/e_capi.c:814:CAPI unknown
engines/e_capi.c:835:CAPI unknown
engines/e_capi.c:867:CAPI unknown
engines/e_capi.c:875:CAPI unknown
engines/e_capi.c:882:CAPI unknown
engines/e_capi.c:890:CAPI unknown
engines/e_capi.c:931:CAPI unknown
engines/e_capi.c:938:CAPI unknown
engines/e_capi.c:945:CAPI unknown
engines/e_capi.c:954:CAPI unknown
engines/e_capi.c:995:CAPI unknown
engines/e_capi.c:1000:CAPI unknown
engines/e_capi.c:1006:CAPI unknown
engines/e_capi.c:1013:CAPI unknown
engines/e_capi.c:1021:CAPI unknown
engines/e_capi.c:1065:CAPI unknown
engines/e_capi.c:1102:CAPI unknown
engines/e_capi.c:1107:CAPI unknown
engines/e_capi.c:1112:CAPI unknown
engines/e_capi.c:1128:CAPI unknown
engines/e_capi.c:1134:CAPI unknown
engines/e_capi.c:1142:CAPI unknown
engines/e_capi.c:1197:CAPI unknown
engines/e_capi.c:1204:CAPI unknown
engines/e_capi.c:1211:CAPI unknown
engines/e_capi.c:1221:CAPI unknown
engines/e_capi.c:1238:CAPI unknown
engines/e_capi.c:1272:CAPI unknown
engines/e_capi.c:1277:CAPI unknown
engines/e_capi.c:1327:CAPI unknown
engines/e_capi.c:1389:CAPI unknown
engines/e_capi.c:1496:CAPI unknown
engines/e_capi.c:1501:CAPI unknown
engines/e_capi.c:1590:CAPI unknown
engines/e_capi.c:1632:CAPI unknown
engines/e_capi.c:1641:CAPI unknown
engines/e_capi.c:1824:CAPI unknown
engines/e_capi.c:1835:CAPI unknown
engines/e_dasync.c:195:DASYNC unknown
engines/e_dasync.c:210:DASYNC unknown
engines/e_dasync.c:641:DASYNC unknown
engines/e_ossltest.c:318:OSSLTEST unknown
crypto/srp/srp_vfy.c:192:SRP unknown

@levitte
Copy link
Member Author

levitte commented Jun 11, 2018

I discovered that long list of errors a few moments ago. Will fix in a new PR, where I will teach the script to take the same config file option as mkerr.pl.

@levitte
Copy link
Member Author

levitte commented Jun 11, 2018

(also, it seems our Travis script is flawed, or this fault would have been discovered before merging)

@DDvO
Copy link
Contributor

DDvO commented Jun 11, 2018

These inconsistencies are likely not the only ones, in particular when new sub-libraries/modules are added, given the rather intricate way they are integrated,in the crypto lib with the various meta files that need to be kept in sync. Well, this is supported by various Perl scripts, yet they have their complexity in themselves and non-trivial interactions also with the Makefile.

Would be nice if sub-library/module integration could be defined and managed more centrally, minimizing redundancies.

@richsalz
Copy link
Contributor

Would be nice if sub-library/module integration could be defined and managed more centrally, minimizing redundancies.

YES! Can you open an issue for this? Let's try to address it in one place with one file.

@DDvO
Copy link
Contributor

DDvO commented Jun 11, 2018

Done.

@DDvO DDvO mentioned this pull request Jun 11, 2018
Open
@levitte levitte mentioned this pull request Jun 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richsalz richsalz richsalz approved these changes

Assignees
No one assigned
Labels
branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@levitte @richsalz @DDvO