Fix master build with --strict-warnings. #20436

FdaSilvaYY · 2023-03-05T21:42:47Z

Found while trying to a CI failure.

~~Improve nmake clean cleanup.~~

Checklist

documentation is added or updated
tests are added or updated

FdaSilvaYY · 2023-03-05T21:51:04Z

Ping @DDvO : feel free to keep my fix or discard all the const in this method.

mspncp · 2023-03-05T22:51:13Z

crypto/cmp/cmp_ctx.c

-                                  int len)
+                                  const int len)


What is the point of marking the parameter len as const in the first place? It's passed by value, so the caller doesn't need to care whether the value is modified inside the function.

In my opinion, it's not the function signature of the implementation which needs to be changed, it's the signature of the prototype in the header and the documentation.

~/src/openssl/master$ git grep -n -A 1 OSSL_CMP_CTX_set1_secretValue | grep -B 1 'int len' crypto/cmp/cmp_ctx.c:421:int OSSL_CMP_CTX_set1_secretValue(OSSL_CMP_CTX *ctx, const unsigned char *sec, crypto/cmp/cmp_ctx.c-422- int len) -- doc/man3/OSSL_CMP_CTX_new.pod:118: int OSSL_CMP_CTX_set1_secretValue(OSSL_CMP_CTX *ctx, const unsigned char *sec, doc/man3/OSSL_CMP_CTX_new.pod-119- const int len); -- include/openssl/cmp.h.in:327:int OSSL_CMP_CTX_set1_secretValue(OSSL_CMP_CTX *ctx, const unsigned char *sec, include/openssl/cmp.h.in-328- const int len);

That's why I ping @DDvO to get its feeling, because this way of fixing it is much larger.
This could be a merge conflict remainder or a review issue.

constifying scalar function parameters is a bit over-zealous, I agree.
I think that in certain places, this is done to indicate an intent that the input parameter will be used unchanged, i.e. will not be "adjusted" frivolously.

In other words: it's leaking implementation details without good cause ;-)

In other words: it's leaking implementation details without good cause ;-)

No, it's making a promise.

Sorry for my somewhat delayed response, I did not notice this PR and question to me before.

I cannot recall how that const int came in originally and see little practical use for it
(except that it may help the implementer of the function not to accidentally modify the parameter value).

As one can easily find out from the commit history, already in commit 08dfbe0 I removed the const,
so please do not re-introduce it here.
BTW, @FdaSilvaYY I just found that you had commented on that PR, though on a different topic: #17284 (comment).

Unfortunately, cmp.h.in still contains

int OSSL_CMP_CTX_set1_secretValue(OSSL_CMP_CTX *ctx, const unsigned char *sec, const int len);

so I presume that you aim to make the function definition in cmp_ctx.c consistent with that.

I'd much prefer if the const instead gets removed from the declaration in the header file,
yet we then face the problem that this appears (syntactically) to be an API change/break,
while actually (semantically) it has no effect on the API (i.e., the interface and behavior for callers).

@DDvO I push a fix in this way.

Configurations/windows-makefile.tmpl

DDvO

Thanks for the cleanup

DDvO

Ah, one more thing: please change the const int also in OSSL_CMP_CTX_new.pod.

A pity that doc-nits cannot check the consistency of (function etc.) type decls with the respective header files.

DDvO · 2023-03-10T07:21:33Z

Regarding the consistency of the documentation, the checklist in the PR text pattern is a nice hint,
but of course easy to overlook when the need for doc changes just comes up while modifying the PR:

Checklist

documentation is added or updated

tests are added or updated

FdaSilvaYY · 2023-03-10T08:10:52Z

Ah, one more thing: please change the const int also in OSSL_CMP_CTX_new.pod.

A pity that doc-nits cannot check the consistency of (function etc.) type decls with the respective header files.

OSSL_CMP_CTX_new.pod fix is in my commit.

This PR is ready to review.

DDvO · 2023-03-10T08:17:25Z

Ah, one more thing: please change the const int also in OSSL_CMP_CTX_new.pod.
A pity that doc-nits cannot check the consistency of (function etc.) type decls with the respective header files.

OSSL_CMP_CTX_new.pod fix is in my commit.

This PR is ready to review.

Oh, I had overlooked this - sorry for the confusion.
So I've renewed my approval.

include/openssl/cmp.h.in

crypto/cmp/cmp_ctx.c

doc/man3/OSSL_CMP_CTX_new.pod

FdaSilvaYY · 2023-03-10T14:23:27Z

@mspncp : changes split in two commits
Please review

DDvO

reconfirmed

remove unneeded const qualifier to keep method declaration and definition in sync.

mspncp · 2023-03-10T20:01:08Z

Last force-push was without tree changes.

@FdaSilvaYY sorry, I couldn't resist: I swapped the two commits: the whitespace reformatting needs to go first (in topological order), otherwise it covers the relevant change in the annotation. Also, I inserted an an empty line between subject line and body in one of the commit messages.

levitte · 2023-03-14T10:47:05Z

I've just run a test, attempting to compile with this code:
#include <openssl/cmp.h>

int main(void)
{
    int (*secretValue)(OSSL_CMP_CTX *ctx, const unsigned char *sec,
                    const int len);

    secretValue = OSSL_CMP_CTX_set1_secretValue;

    secretValue(NULL, NULL, 0);
    return 0;
}
Compiled with -Wall using both gcc (11.3) and clang (14.0) and against a version of OpenSSL which has dropped the const for the len parameter from the header. I was unable to get either to issue a warning about this. Not a guarantee for all compilers of course.

This search indicates that clang 15 is currently packaged in Debian [testing], which will become the next stable release:

https://packages.debian.org/search?keywords=clang

Looking at Debians release history and cadence (https://wiki.debian.org/DebianReleases), I would expect Debian 12 (which is what Debian [testing] is right now) to be released some time this year...
So, I'm wondering if it might be a good idea to try with that clang version.

Disclaimer: I'm an not a Debian developer / maintainer

levitte · 2023-03-14T10:51:43Z

Glad to see that at least current gcc and clang have no issue here.

The latest releases for these are gcc 12 and clang 15, according to their own release pages:

https://gcc.gnu.org/releases.html
https://releases.llvm.org/

I can try with those

levitte · 2023-03-14T11:01:02Z

No problem with gcc-12 or clang-15 either:

/* I did it a little differently for demonstration purposes */
#include <stddef.h>
/* #include <openssl/cmp.h> */
typedef struct ossl_cmp_ctx_st OSSL_CMP_CTX;
int OSSL_CMP_CTX_set1_secretValue(OSSL_CMP_CTX *ctx, const unsigned char *sec,
                                  int len);

int main(void)
{
    int (*secretValue)(OSSL_CMP_CTX *ctx, const unsigned char *sec,
                       const int len);

    secretValue = OSSL_CMP_CTX_set1_secretValue;

    secretValue(NULL, NULL, 0);
    return 0;
}

$ gcc-12 -Wall -o 20436.o -c 20436.c
$ clang-15 -Wall -o 20436.o -c 20436.c
$

This confirms the "not a problem" standing as far as I can take it.
... and this has me think that we should really fix the header, and do it all the way to 3.0.

mattcaswell · 2023-03-14T11:02:58Z

Could someone try with VisualStudio? If its not a problem there either then I'm also starting to lean towards "do it all the way to 3.0".

hlandau · 2023-03-14T11:10:11Z

It is not a problem on clang 15 or GCC or MSVC, both for C and C++/extern C.

I am trying to find a reference in the ISO C spec.

hlandau · 2023-03-14T11:31:03Z

C11 s. 6.7.6.3. p. 15. "(In the determination of type compatibility and of a composite type, each parameter [...] declared with qualified type is taken as having the unqualified version of its declared type.)"

C89 s. 6.5.4.3. "For two function types to be compatible, [...] (For each parameter declared with qualified type, its type for these comparisons is the unqualified version of its declared type.)"

hlandau · 2023-03-14T11:34:28Z

Looks like there should be no issue doing the header change, e.g. in 3.0, both in theory and in practice.

DDvO · 2023-03-14T11:45:52Z

I suggest clarifying this once and for all, not only for this PR, and sharpening the OpenSSL definition/understanding of an API break accordingly.

BTW, where can this definition be found?

Is there a clear official OpenSSL definition of
what exactly constitutes an API break, and how to determine this syntactically?

FdaSilvaYY · 2023-03-14T13:17:15Z

@FdaSilvaYY where do you see the warning, which compiler/OS?

I was using Visual 2015, with --strict-warnings, to debug a build break of my another PR.

DDvO · 2023-03-14T13:58:35Z

@FdaSilvaYY where do you see the warning, which compiler/OS?

I was using Visual 2015, with --strict-warnings, to debug a build break of my another PR.

Ah yeah, MickeySoft always tends to make trouble, with all sorts of incompatibility.

paulidale · 2023-03-15T01:07:10Z

Fix the header and merge back to 3.0 seems to be the consensus the group is coming to.
I can support this.

t8m · 2023-03-21T08:17:23Z

OTC: Given the compilers ignore that const we want to fix the header and backport that to 3.1 and 3.0 branches.

t8m · 2023-03-21T08:18:48Z

If this does not apply cleanly we will need a backport patch for 3.1.

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #20436)

remove unneeded const qualifier to keep method declaration and definition in sync. Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #20436)

t8m · 2023-03-21T10:01:54Z

Merged to master, 3.1 and 3.0 branches. Thank you for your contribution.

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #20436) (cherry picked from commit f42d6b7)

remove unneeded const qualifier to keep method declaration and definition in sync. Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #20436) (cherry picked from commit 6f792f4)

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #20436) (cherry picked from commit f42d6b7)

remove unneeded const qualifier to keep method declaration and definition in sync. Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #20436) (cherry picked from commit 6f792f4)

DDvO · 2023-03-21T10:14:31Z

@openssl/otc, again my questions:

I suggest clarifying this once and for all, not only for this PR, and sharpening the OpenSSL definition/understanding of an API break accordingly.
BTW, where can this definition be found?

Is there a clear official OpenSSL definition of what exactly constitutes an API break, and how to determine this syntactically?

At least the outcome of our above exchange should be remembered somehow
such that on any future such cases it does not need to get re-discovered with effort.

t8m · 2023-03-21T10:49:36Z

The problem is that basically anything that changes in the public API headers that can potential break a compilation for anyone is an API break. So until we found that this is very unlikely to break anyone it was considered an API break. Not sure how to record this and where.

DDvO · 2023-03-21T11:21:09Z

The problem is that basically anything that changes in the public API headers that can potential break a compilation for anyone is an API break. So until we found that this is very unlikely to break anyone it was considered an API break. Not sure how to record this and where.

Thanks for your response, which more or less includes a definition of an API break.
Unfortunately, it won't be easy to give simple syntactic rules to correctly and completely determine what will constitute an API break or not.

For instance, the following header file changes do not lead to API breaks:

whitespace and comment changes
adding or removing the const qualifier before a primitive (i.e., non-reference) type
adding or removing the name or changing the name of a function parameter
adding a declaration
...?

I suggest adding something like this to the technical policies, next to the coding style.

t8m · 2023-03-21T11:51:50Z

Added openssl/technical-policies#65

mspncp requested changes Mar 5, 2023

View reviewed changes

levitte requested changes Mar 6, 2023

View reviewed changes

Configurations/windows-makefile.tmpl Outdated Show resolved Hide resolved

Configurations/windows-makefile.tmpl Outdated Show resolved Hide resolved

Configurations/windows-makefile.tmpl Outdated Show resolved Hide resolved

Configurations/windows-makefile.tmpl Outdated Show resolved Hide resolved

FdaSilvaYY force-pushed the 2022/03/Master-Cmp-Fixup branch from a50ae45 to 57f3279 Compare March 9, 2023 21:30

DDvO approved these changes Mar 10, 2023

View reviewed changes

DDvO requested a review from mspncp March 10, 2023 07:09

DDvO added branch: master Merge to master branch approval: otc review pending This pull request needs review by an OTC member triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly labels Mar 10, 2023

DDvO requested changes Mar 10, 2023

View reviewed changes

DDvO added the approval: review pending This pull request needs review by a committer label Mar 10, 2023

DDvO self-requested a review March 10, 2023 08:15

DDvO approved these changes Mar 10, 2023

View reviewed changes

DDvO added tests: exempted The PR is exempt from requirements for testing triaged: documentation The issue/pr deals with documentation (errors) and removed approval: review pending This pull request needs review by a committer labels Mar 10, 2023

mspncp requested changes Mar 10, 2023

View reviewed changes

include/openssl/cmp.h.in Show resolved Hide resolved

crypto/cmp/cmp_ctx.c Show resolved Hide resolved

doc/man3/OSSL_CMP_CTX_new.pod Show resolved Hide resolved

FdaSilvaYY force-pushed the 2022/03/Master-Cmp-Fixup branch from 57f3279 to 003db5c Compare March 10, 2023 14:22

DDvO approved these changes Mar 10, 2023

View reviewed changes

FdaSilvaYY added 2 commits March 10, 2023 20:46

nit: tidy-up code, and fix a typo.

70f39ad

cmp: fix --strict-warnings windows builds

c7214af

remove unneeded const qualifier to keep method declaration and definition in sync.

mspncp force-pushed the 2022/03/Master-Cmp-Fixup branch from 003db5c to c7214af Compare March 10, 2023 19:57

mspncp approved these changes Mar 10, 2023

View reviewed changes

mspncp added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Mar 10, 2023

t8m added branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 and removed hold: need otc decision The OTC needs to make a decision labels Mar 21, 2023

openssl-machine pushed a commit that referenced this pull request Mar 21, 2023

nit: tidy-up code, and fix a typo.

f42d6b7

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #20436)

t8m closed this Mar 21, 2023

t8m mentioned this pull request Mar 21, 2023

Clarify what constitutes API break and what not openssl/technical-policies#65

Open

FdaSilvaYY deleted the 2022/03/Master-Cmp-Fixup branch March 25, 2023 00:29

Fix master build with --strict-warnings. #20436

Fix master build with --strict-warnings. #20436

Conversation

FdaSilvaYY commented Mar 5, 2023 • edited

Checklist

FdaSilvaYY commented Mar 5, 2023

mspncp Mar 5, 2023 • edited

Choose a reason for hiding this comment

FdaSilvaYY Mar 5, 2023

Choose a reason for hiding this comment

levitte Mar 6, 2023

Choose a reason for hiding this comment

mspncp Mar 6, 2023

Choose a reason for hiding this comment

levitte Mar 6, 2023

Choose a reason for hiding this comment

DDvO Mar 9, 2023

Choose a reason for hiding this comment

FdaSilvaYY Mar 9, 2023

Choose a reason for hiding this comment

DDvO left a comment

Choose a reason for hiding this comment

DDvO left a comment

Choose a reason for hiding this comment

DDvO commented Mar 10, 2023 • edited

Checklist

FdaSilvaYY commented Mar 10, 2023

DDvO commented Mar 10, 2023 • edited

FdaSilvaYY commented Mar 10, 2023

DDvO left a comment

Choose a reason for hiding this comment

mspncp commented Mar 10, 2023

levitte commented Mar 14, 2023 • edited

levitte commented Mar 14, 2023

levitte commented Mar 14, 2023

mattcaswell commented Mar 14, 2023

hlandau commented Mar 14, 2023 • edited

hlandau commented Mar 14, 2023 • edited

hlandau commented Mar 14, 2023

DDvO commented Mar 14, 2023

FdaSilvaYY commented Mar 14, 2023 • edited

DDvO commented Mar 14, 2023

paulidale commented Mar 15, 2023

t8m commented Mar 21, 2023

t8m commented Mar 21, 2023

t8m commented Mar 21, 2023

DDvO commented Mar 21, 2023 • edited

t8m commented Mar 21, 2023

DDvO commented Mar 21, 2023 • edited

t8m commented Mar 21, 2023

FdaSilvaYY commented Mar 5, 2023 •

edited

mspncp Mar 5, 2023 •

edited

DDvO commented Mar 10, 2023 •

edited

DDvO commented Mar 10, 2023 •

edited

levitte commented Mar 14, 2023 •

edited

hlandau commented Mar 14, 2023 •

edited

hlandau commented Mar 14, 2023 •

edited

FdaSilvaYY commented Mar 14, 2023 •

edited

DDvO commented Mar 21, 2023 •

edited

DDvO commented Mar 21, 2023 •

edited