Fix potentially uninitialised pointer

[kinkie]

Newer compilers barf on an uninitialised pointer in
src/security/Handshake.cc . Fix it.

found a better solution I think

[yadij]

Can you check if replacing "(void)pCert;" with "pCert = nullptr;" in the #else condition of ParseCertificate also fixes these issues. That would be better if is works.
The intended API is that the pointer is unset going into the function and set coming out.

[kinkie]

Yes, changing that bit also works.
I'd argue that explicitly initializing it clarifies intent better though; it may make sense to do both, what do you think?

[yadij]

it is a smart pointer except in that particular #else code path. so using the function to initialize is fine and avoids a double-initialization in the other paths which are going to become more common.

[rousskov]

If the fix is not difficult, then we should fix the core problem (as defined below) rather than working around the core problem in one specific location.

An invariant we want to maintain: In all builds, all code outside of library-specific #ifdefs, should be given the same library-agnostic CertPointer API.

Core problem: The current code violates the above invariant. One known violation is that we get uninitialized pointers in some builds (because void* lacks default initialization while Security::Pointer has it and, naturally, some library-agnostic code relies on that default initialization).

If you have not already, please try to find a CertPointer type other than void * that would work and not require explicit initialization. For example, std::nullptr_t, an enum or something similar to an empty class with an explicit boolean operator might work.

If you have done that already, please discuss why nothing can replace void *.

[yadij]

I have tried to find one last year before this type was used. The number of constraints make it quite difficult to use a smart-pointer. It has to be defined in a typedef without compiler issues and capable of implicit conversion to nullptr, NULL, and boolean - smart-pointer foo and foo<nullptr_t> cannot do all of those on all the compilers we need to support, and using a non-void/nullptr_t type results in unused variable issues (the "(void)pCert;" hack was to fix one of those originally) within the std:: template expansions in various places that were not able to be resolved with #else hacks.

My suggested fix for the current build issue makes the ParseCertificate behaviour and API identical regardless of whether a smart Pointer or raw-pointer is received. It come in unset and goes out set to a relevant value. That #else code using a smart-pointer would have to change in exactly the same way. It is not a fix for the void* invariant, but fixes/erases the broken hack in the currently failing code path.

[kinkie]

I have tried to find one last year before this type was used. The number of constraints make it quite difficult to use a smart-pointer. It has to be defined in a typedef without compiler issues and capable of implicit conversion to nullptr, NULL, and boolean - smart-pointer foo and foo<nullptr_t> cannot do all of those on all the compilers we need to support, and using a non-void/nullptr_t type results in unused variable issues (the "(void)pCert;" hack was to fix one of those originally) within the std:: template expansions in various places that were not able to be resolved with #else hacks. My suggested fix for the current build issue makes the ParseCertificate behaviour and API identical regardless of whether a smart Pointer or raw-pointer is received. It come in unset and goes out set to a relevant value. That #else code using a smart-pointer would have to change in exactly the same way. It is not a fix for the void* invariant, but fixes/erases the broken hack in the currently failing code path.

It seems to be working. Mind approving the pull request? Thanks! Francesco

[yadij]

Approved. Though I do agree with Alex we need to fix the deeper issues long-term, I think that can be fixed separately to the immediate issue / scope.

[kinkie]

Hm.. Alex needs to retract his request for changes before merge can happen.
I'm quite unsure how we should deal with these situations. Would dismissing the change request review be acceptable?

[rousskov]

Would dismissing the change request review be acceptable?

Dismissing core reviewer requests is not acceptable (except in emergencies). You need to convince the blocking reviewer to dismiss or change the vote. I am surprised you had to ask, so perhaps I am missing something.

[rousskov]

The number of constraints make it quite difficult to use a smart-pointer.

I would like to understand/agree with that before withdrawing/changing my vote.

It has to be defined in a typedef

Agreed.

capable of implicit conversion to nullptr, NULL, and boolean

That does not sound quite difficult. After all, all standard smart pointers do that with ease, right?

smart-pointer foo and foo<nullptr_t>

I am not sure what you meant by that part.

on all the compilers we need to support

I assume we can use C++11 but please let me know if my assumption is wrong.

I usually regret sketching specific solutions, but since I see no reasons why something simple would not work, I will do that in hope to make progress. Let's start with arguably the most "obvious" ones:

// 1. Use Security::LockingPointer itself
inline void DoNothingWithNothing(void*) {}
typedef Security::LockingPointer<void, DoNothingWithNothing> CertPointer;

// 2. Use a standard smart pointer
#include <memory>
typedef std::shared_ptr<void> CertPointer;

[rousskov]

My suggested fix for the current build issue makes the ParseCertificate behaviour and API identical regardless of whether a smart Pointer or raw-pointer is received

Agreed. Whether ParseCertificate() API is correct or not is a separate question. The proposed fix just works with the existing API. There is nothing wrong with that, fixing that API is out of scope here, and the proposed change is probably a good idea even if we fix the CertPointer declaration. Thus, this PR should be merged.

The build failure exposed CertPointer declaration bug. However, since the proposed change may still be a good idea after we fix the CertPointer declaration, we must not hold this PR hostage. I hope somebody will volunteer to fix the CertPointer (and other Security pointers) declarations, but this PR is not the place to track that work.

---

AFAICT, the problems with the ParseCertificate API started in commit a34d1d2. Before that change, Squid did not attempt to parse certificates when there was no support for doing so and, hence, did not add nil pointers to the parsed certificate list. @chtsanti, do we need to store nil certificate pointers when we cannot parse certificates? And do you remember why we moved USE_OPENSSL guards in commit a34d1d2?

I am dismissing my review because the proposed changes are probably needed even if the bigger issue of CertPointer declarations is fixed.

[rousskov]

@kinkie, technically, you should also remove the workaround GCC -O3 error comment because pointer initialization is the right thing to do even if GCC remains silent. I am not formally requesting that change because it is minor, and you probably suffered too much with this PR already :-). I did not click the "Squash and merge" button myself to give you a chance to consider comment removal.

[kinkie]

On Aug 7, 2017, at 19:49, Alex Rousskov ***@***.***> wrote: Would dismissing the change request review be acceptable? Dismissing core reviewer requests is not acceptable (except in emergencies). You need to convince the blocking reviewer to dismiss or change the vote. I am surprised you had to ask, so perhaps I am missing something.

Maybe I am; there is one negative vote and two positives; it’s an uncommon enough situation that I had to review the merge guidelines (after posting this). I was in doubt that github was correctly enforcing our rules, it just exposed that fact that I didn’t remember the rules :) Kinkie

[rousskov]

FWIW, GitHub does not know about our Project rules -- its voting software is too simplistic to fully support them. On the other hand, both GitHub algorithms and our rules are more-or-less reasonable, so there is a significant overlap that allows us to vote on GitHub without creating major problems, at least until there are more than a handful of (same-weight) people actually voting.

I am still surprised you think that simply dismissing a blocking (core) developer vote is a good idea (rules or no rules). Software development is not a popularity contest where the highest vote getter wins -- negative votes are meant to protect the official code; dismissing them just because there are more positive votes does not make sense IMO.

[rousskov]

@kinkie, please do not merge internal branch commits unless they are very valuable. Squash your changes first. See item 5 in the Pull Request section of the Merge Procedure.

To minimize polluting commits, I am going to disable non-squashed merges for now. They can be enabled in those rare cases where a PR needs them, of course. Eventually, the right merge method will be selected automatically during the automated merge, but we are still quite far from that state AFAICT.

[kinkie]

On 7 Aug 2017, at 23:52, Alex Rousskov ***@***.***> wrote: @kinkie <https://github.com/kinkie>, please do not merge internal branch commits unless they are very valuable. Squash your changes first. See item 5 in the Pull Request <https://wiki.squid-cache.org/MergeProcedure#Pull_Request> section of the Merge Procedure <https://wiki.squid-cache.org/MergeProcedure>. To minimize polluting commits, I am going to disable non-squashed merges for now. They can be enabled in those rare cases where a PR needs them, of course. Eventually, the right merge method will be selected automatically during the automated merge, but we are still quite far from that state AFAICT.

Ok, will do. Thanks for the advice

[chtsanti]

AFAICT, the problems with the ParseCertificate API started in commit a34d1d2. Before that change, Squid did not attempt to parse certificates when there was no support for doing so and, hence, did not add nil pointers to the parsed certificate list. @chtsanti, do we need to store nil certificate pointers when we cannot parse certificates? And do you remember why we moved USE_OPENSSL guards in commit a34d1d2?

The code is problematic here. Storing nil certificate pointers is not correct and we may have problems in other cases where the code expects a certificate in list and does not check for nil.

This code appeared as part of fetch certificates patch, but probably applied as separate patch to trunk.

We must fix it.

[yadij]

@christos, the nil value on these Pointers is guaranteed by libsecurity in any case where the cert is not able to be loaded (ie. no library support). Code which has been converted to remove #if USE_OPENSSL should be able to cope with nil (if not that is a separate bug which needs fixing), and not-ParseCertificate code which is wrapped with USE_OPENSSL should be provided with a cert, or handles nil as part of the ParseCertificate Must(x509) handling pathway.

FTR the #else code in question was part of the TidyPointer removal for libsecurity API updates to Handshake.cc

[chtsanti]

@yadij The problematic code is inside Security::HandshakeParser::parseServerCertificates:

while (!tkItems.atEnd()) {
      Security::CertPointer cert;
      ParseCertificate(tkItems.pstring24("Certificate"), cert);
      serverCertificates.push_back(cert);
}

In the case the USE_OPENSSL is not defined we are filling the serverCertificates list with nil values. Even if now there is not any bug, because this list is used only by openSSL related code it is possible that it will be cause problems in the future. The code must be changed at least to do something like the following:

while (!tkItems.atEnd()) {
        Security::CertPointer cert;
        ParseCertificate(tkItems.pstring24("Certificate"), cert);
        if (cert != nullptr)
            serverCertificates.push_back(cert);
  }

I am not saying that this is should fixed into this PR. I just answered Alex question:

...did not add nil pointers to the parsed certificate list. @chtsanti, do we need to store nil certificate pointers when we cannot parse certificates?...

[rousskov]

@chtsanti, glad you think that we do not want nil pointers in the parsed certificate list! Please fix the code accordingly:

1. ParseCertificate() must either throw or return a non-nil pointer to the parsed certificate.
2. Outside of USE_OPENSSL, do not call ParseCertificate() and do not iterate over extracted certificate bytes (partially undoing a34d1d2 changes). Consider making parseServerCertificates() itself a no-op (undoing even more a34d1d2 changes).

a34d1d2 appeared as part of fetch certificates patch, but probably applied as separate patch to trunk.

Exactly. I am still a little uncomfortable that we do not know why we thought it is OK (or even required) to store nil pointers in the certificate list, but I will go with your assessment that it was just an accident.

All of the above has nothing to do with this PR. I am just documenting the conclusion of the above discussion so that @yadij can review and object if needed before you change code. This discussion probably belongs to squid-dev instead. Sorry for the noise!