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.

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

Man pages provide synopses of numerous BN_* methods that are inconsistent with their header declarations #19521

Open
dimitrijekostic opened this issue Oct 27, 2022 · 3 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 good first issue Bite size change that could be a good start triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors)

Comments

@dimitrijekostic
Copy link

The OpenSSL man pages (both in the repo here and on openssl.org) provide inaccurate descriptions of several of the arithmetic operations on BIGNUMs. The errors I've noticed all involve missing const qualifiers on function arguments.

Below are several BN_* function declarations given in the man pages (drawn mostly from master/doc/man3/BN_add.pod but also a few others) and also from the online documentation for OpenSSL 3.1 (drawn mostly from this page but also a few others):

  1. int BN_mul(BIGNUM *r, BIGNUM *a, BIGNUM *b, BN_CTX *ctx);
  2. int BN_sqr(BIGNUM *r, BIGNUM *a, BN_CTX *ctx);
  3. int BN_mod_add(BIGNUM *r, BIGNUM *a, BIGNUM *b, const BIGNUM *m, BN_CTX *ctx);
  4. int BN_mod_sub(BIGNUM *r, BIGNUM *a, BIGNUM *b, const BIGNUM *m, BN_CTX *ctx);
  5. int BN_mod_mul(BIGNUM *r, BIGNUM *a, BIGNUM *b, const BIGNUM *m, BN_CTX *ctx);
  6. int BN_mod_sqr(BIGNUM *r, BIGNUM *a, const BIGNUM *m, BN_CTX *ctx);
  7. BIGNUM *BN_mod_sqrt(BIGNUM *in, BIGNUM *a, const BIGNUM *p, BN_CTX *ctx);
  8. int BN_exp(BIGNUM *r, BIGNUM *a, BIGNUM *p, BN_CTX *ctx);
  9. int BN_mod_exp(BIGNUM *r, BIGNUM *a, const BIGNUM *p, const BIGNUM *m, BN_CTX *ctx);
  10. int BN_gcd(BIGNUM *r, BIGNUM *a, BIGNUM *b, BN_CTX *ctx);
  11. int BN_lshift1(BIGNUM *r, BIGNUM *a);
  12. int BN_rshift(BIGNUM *r, BIGNUM *a, int n);
  13. int BN_rshift1(BIGNUM *r, BIGNUM *a);
  14. BIGNUM *BN_mod_inverse(BIGNUM *r, BIGNUM *a, const BIGNUM *n, BN_CTX *ctx);

These declarations are puzzling at first glance because they do not const protect arguments that, according to the descriptions, do not need to be modified during a call.

Below are the respective function declarations given in master/include/openssl/bn.h, in which function arguments are appropriately const-qualified.

  1. int BN_mul(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, BN_CTX *ctx);
  2. int BN_sqr(BIGNUM *r, const BIGNUM *a, BN_CTX *ctx);
  3. int BN_mod_add(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, const BIGNUM *m, BN_CTX *ctx);
  4. int BN_mod_sub(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, const BIGNUM *m, BN_CTX *ctx);
  5. int BN_mod_mul(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, const BIGNUM *m, BN_CTX *ctx);
  6. int BN_mod_sqr(BIGNUM *r, const BIGNUM *a, const BIGNUM *m, BN_CTX *ctx);
  7. BIGNUM *BN_mod_sqrt(BIGNUM *ret, const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx);
  8. int BN_exp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, BN_CTX *ctx);
  9. int BN_mod_exp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, const BIGNUM *m, BN_CTX *ctx);
  10. int BN_gcd(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, BN_CTX *ctx);
  11. int BN_lshift1(BIGNUM *r, const BIGNUM *a);
  12. int BN_rshift(BIGNUM *r, const BIGNUM *a, int n);
  13. int BN_rshift1(BIGNUM *r, const BIGNUM *a);
  14. BIGNUM *BN_mod_inverse(BIGNUM *ret, const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx);

These errors are in the master branch for version 3.1, but appear to have been carried over from older versions of OpenSSL. I suggest that the man pages for all versions of OpenSSL (or, at least, the currently-supported ones) be updated with the correct function declarations.

There may also be other similar errors elsewhere in the documentation; these are just the ones that leapt out at me. I haven't made a comprehensive search for discrepancies, but I suggest you do so.

@dimitrijekostic dimitrijekostic added the issue: documentation The issue reports errors in (or missing) documentation label Oct 27, 2022
@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 good first issue Bite size change that could be a good start and removed issue: documentation The issue reports errors in (or missing) documentation labels Oct 27, 2022
@levitte
Copy link
Member

levitte commented Oct 28, 2022

Thank you, that's indeed documentation updates we failed to make.

@mattcaswell
Copy link
Member

Thank you, that's indeed documentation updates we failed to make.

I wonder if we can somehow extend doc-nits to check for consistency of man page declarations with the equivalent declarations in the header files?

@levitte
Copy link
Member

levitte commented Oct 31, 2022

Thank you, that's indeed documentation updates we failed to make.

I wonder if we can somehow extend doc-nits to check for consistency of man page declarations with the equivalent declarations in the header files?

Hmmm... we might be able to extract the source from synopsises into a temporary C file with a main() that does nothing, and try to compile it, leaving it to the compiler to diagnose any discrepancies. That actually doesn't sound like a very difficult task, albeit a tedious one.

EDIT: I've added an issue with this idea: #19549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 good first issue Bite size change that could be a good start triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

No branches or pull requests

4 participants