mozilla::pkix always uses the default timeout for OCSP in EV and not 10 seconds

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cviecco, Assigned: keeler)

Tracking

unspecified
mozilla32
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31+ fixed, firefox32 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

5 years ago
Currently the OCSP timeout for ev is 10 seconds (libpkix default). In insanity since we call CERT_PostOCSPRequest we use the default nss timeout which is set to 2 seconds ( bug 918120) thus we have to be in a very good network to get EV treatment in insanity.
Reporter

Updated

5 years ago
Summary: Insanity::pkix always uses the default timeout for OCSP in EV and not 10 seconds → mozilla::pkix always uses the default timeout for OCSP in EV and not 10 seconds
Assignee: nobody → brian
Posted patch patch (obsolete) — Splinter Review
For this to work, we need a way to specify the timeout on a per-request basis. I looked at the ~700 lines of code in PSM that implements OCSP requests, and I think it could be simplified to something like this. There are a few features it doesn't support (like preventing an upgrade to https), but they could easily be added in. Also, to switch to using something like this full-time, we would need to avoid all main-thread verification, which is currently of lower priority than getting mozilla::pkix shipped.
Attachment #8403564 - Flags: feedback?(cviecco)
Comment on attachment 8403564 [details] [diff] [review]
patch

Review of attachment 8403564 [details] [diff] [review]:
-----------------------------------------------------------------

David, the overall idea of doing the OCSP request in Gecko instead of in NSS is good.

However, I think there may already be a simplified API for doing a synchronous POST with a specified timeout (much more like XMLHTTPRequest) that would be much less code. Please check with others about that.

::: security/certverifier/OCSPRequestor.cpp
@@ +223,5 @@
> +  if (NS_IsMainThread()) {
> +    return CERT_PostOCSPRequest(arena, url, encodedRequest);
> +  }
> +  nsCOMPtr<nsIRunnable> request(new OCSPRequest(arena, url, encodedRequest));
> +  return ((OCSPRequest*)request.get())->DoOCSPRequest(timeoutSeconds); // XXX arg, what's the right way to do this?

I think this is what SyncRunnableBase is useful for. Otherwise, I am not understanding your question.

BTW, I think you could modify SyncRunnableBase to use Wait(PRIntervalTime) to wait for up to 2/10 seconds to eliminate the use of the timer. Otherwise, I might not be understanding your question here.
Attachment #8403564 - Flags: feedback?(brian) → feedback+
Assignee: brian → dkeeler
Reporter

Comment 3

5 years ago
Comment on attachment 8403564 [details] [diff] [review]
patch

Review of attachment 8403564 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +297,5 @@
>      return SECFailure;
>    }
>  
> +  const SECItem* response(DoOCSPRequest(arena.get(), url.get(), request,
> +                            mOCSPFetching == FetchOCSPForEV ? 10 : 2));

It is also 10 secs when ocsprequired is enabled.
Attachment #8403564 - Flags: feedback?(cviecco) → feedback+
Thanks for the feedback.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #2)
> However, I think there may already be a simplified API for doing a
> synchronous POST with a specified timeout (much more like XMLHTTPRequest)
> that would be much less code. Please check with others about that.

I asked around, and the suggestion was to use nsHTTPDownloadEvent, which is basically what I'm trying to avoid. I also attempted to use XMLHttpRequest directly, but ran into difficulties there.

> ::: security/certverifier/OCSPRequestor.cpp
> @@ +223,5 @@
> > +  if (NS_IsMainThread()) {
> > +    return CERT_PostOCSPRequest(arena, url, encodedRequest);
> > +  }
> > +  nsCOMPtr<nsIRunnable> request(new OCSPRequest(arena, url, encodedRequest));
> > +  return ((OCSPRequest*)request.get())->DoOCSPRequest(timeoutSeconds); // XXX arg, what's the right way to do this?
> 
> I think this is what SyncRunnableBase is useful for. Otherwise, I am not
> understanding your question.

Using RefPtr fixed my problem.

> BTW, I think you could modify SyncRunnableBase to use Wait(PRIntervalTime)
> to wait for up to 2/10 seconds to eliminate the use of the timer. Otherwise,
> I might not be understanding your question here.

I re-did how the timer works in a way that I think avoids doing extra synchronization work (which would be required if I did things this way).
Posted patch patch v2 (obsolete) — Splinter Review
Attachment #8403564 - Attachment is obsolete: true
Attachment #8406483 - Flags: review?(brian)
Reporter

Comment 6

5 years ago
Comment on attachment 8406483 [details] [diff] [review]
patch v2

Review of attachment 8406483 [details] [diff] [review]:
-----------------------------------------------------------------

Only one nit in the implementation and something to change on the tests.

::: security/certverifier/OCSPRequestor.cpp
@@ +235,5 @@
> +
> +SECItem* DoOCSPRequest(PLArenaPool* arena, const char* url,
> +                       const SECItem* encodedRequest, uint32_t timeoutSeconds)
> +{
> +  if (NS_IsMainThread()) {

Nit: Mark this section with a bug to do, and print a warning (this is really bad)

::: security/manager/ssl/tests/unit/test_ocsp_timeout.js
@@ +30,5 @@
> +  });
> +
> +  add_connection_test("ocsp-stapling-none.example.com",
> +                      getXPCOMStatusFromNSS(SEC_ERROR_OCSP_SERVER_ERROR),
> +                      clearSessionCache);

I dont see how you are testing the timeout values.
Attachment #8406483 - Flags: review?(cviecco) → review-
Posted patch patch v3 (obsolete) — Splinter Review
Attachment #8406483 - Attachment is obsolete: true
Attachment #8406483 - Flags: review?(brian)
Attachment #8407103 - Flags: review?(cviecco)
Reporter

Updated

5 years ago
Attachment #8407103 - Flags: review?(cviecco) → review+
Comment on attachment 8407103 [details] [diff] [review]
patch v3

Review of attachment 8407103 [details] [diff] [review]:
-----------------------------------------------------------------

The main problem I see with this code is that it duplicates a lot of what is already done in nsHTTPDownloadEvent. The duplication is excessive and we have no clear path to removing the duplication because of the unfortunate need to support the horrible main-thread behavior; this is something that I overlooked before. Also, the duplication is incomplete. Note how nsHTPDownloadEvent sets the channel's priority & load group, how it disables SPDY, etc. If we're already getting the two implementations out of sync now, then it seems hopeless to keep them in sync later. Also, if we wanted to have two implementations then we should at least put them together in the same file, and try to share as much code between them as we can.

Note that fetchOcspHttpClientV1 in ocsp.c, which CERT_PostOCSPRequest ultimately calls, is a very simply function. If we don't want to do OCSP fetching through NSS then we could easily copy/paste/modify fetchOcspHttpClientV1 to get the desired semantics (createFcn takes the timeout as a parameter) with much less duplication, much less work, and with much less chance of something going wrong (e.g. differences in redirect behavior caused by the loadgroup changes or the threading changes). Doing that would better match the scope of the change we actually need to solve this bug.

AFAICT, the way that the code in nsNSSCallbacks.cpp waits for the condition variable to be set is also better (simpler, at least) than the way you've done it with the timer.

Originally I gave feedback+ on this patch because I thought that this would eventually be a straight replacement for the old code in nsNSSCallbacks.cpp. However, I underestimated how much could potentially go wrong or be unintentionally different. If you intended to make semantics changes beyond making the timeout configurable then I suggest filing a separate follow-up bug for making those changes. I'm sorry about my earlier oversight since it seems to have taken a lot of your time.

Below, I included my line-by-line review comments that I wrote before I compared this implementation to the implementation in nsNSSCallbacks.cpp.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +300,5 @@
> +  uint32_t timeoutSeconds = 2;
> +  if (mOCSPFetching == FetchOCSPForEV ||
> +      mOCSPFetching == FetchOCSPForDVHardFail) {
> +    timeoutSeconds = 10;
> +  }

It would be better to have a switch that explicitly maps every value with a default: MOZ_CRASH("not reached"). Any time we add a new value to this enum, we need to consider what the timeout should be.

::: security/certverifier/OCSPRequestor.cpp
@@ +32,5 @@
> +  , timeoutSeconds(timeoutSeconds)
> +  , responseStatus(0)
> +  , channel(nullptr)
> +  , timer(nullptr)
> +  , dataBuffer()

Indention: lines from ":" to here should be indented two more spaces.

@@ +36,5 @@
> +  , dataBuffer()
> +  {
> +  }
> +
> +  virtual ~OCSPRequest();

Why do we need to explicitly declare this empty constructor? AFAICT the compiler-generated constructor should be fine.

@@ +50,5 @@
> +private:
> +  nsresult NotifyDone(nsresult status);
> +
> +  mozilla::Monitor monitor;
> +  PLArenaPool* arena;            // used off the main thread only

The two variables above are also set in the constructor. I'm not sure what value the "set in constructor" comments add (I would just remove them) but at least let's be consistent.

@@ +104,5 @@
> +  uint32_t bytesRead;
> +  nsresult rv = inputStream->Read(writePtr + offset, count, &bytesRead);
> +  if (NS_FAILED(rv)) {
> +    return NotifyDone(rv);
> +  }

You need to check that bytesRead == count since Read() may return NS_OK with bytesRead < count, IIUC. (In theory you may need to call this in a loop until bytesRead == 0).

@@ +110,5 @@
> +}
> +
> +nsresult
> +OCSPRequest::NotifyDone(nsresult status)
> +{

I think you should have a MOZ_ASSERT(NS_IsMainThread()) here since, IIRC, timers have to be destroyed on the same thread they were created on.

@@ +111,5 @@
> +
> +nsresult
> +OCSPRequest::NotifyDone(nsresult status)
> +{
> +  if (channel && NS_SUCCEEDED(status)) {

I think this should be:
MOZ_ASSERT(NS_FAILED(status) || channel);
if (NS_SUCCEEDED(status)) {
  if (!channel) {
    status = <some error code>;
  } else {
    <what you wrote in this block>
  }
}

@@ +115,5 @@
> +  if (channel && NS_SUCCEEDED(status)) {
> +    nsresult rv;
> +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel, &rv);
> +    if (NS_SUCCEEDED(rv) && httpChannel) {
> +      httpChannel->GetResponseStatus(&responseStatus);

Check the return value of GetResponseStatus or mark it [[infalable]], in which case you can rewrite this to responseStatus = httpChannel->GetResponseStatus();

@@ +129,5 @@
> +}
> +
> +NS_IMETHODIMP
> +OCSPRequest::Run()
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +133,5 @@
> +{
> +  nsresult rv;
> +  timer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +  if (!timer) {
> +    return NotifyDone(NS_ERROR_FAILURE);

If you use the two-argument form of do_CreateInstance(NS_TIMER_CONTRACTID, &rv) then you can return NotifyDone(rv);

@@ +203,5 @@
> +  }
> +  rv = lock.Wait();
> +  if (NS_FAILED(rv)) {
> +    return nullptr;
> +  }

The above code is exactly SyncRunnableBase::DispatchToMainThreadAndWait except SyncRunnableBase::DispatchToMainThreadAndWait has special logic to handle the case where it called on the main thread. It is much preferable to use SyncRunnableBase than to re-implement a custom version of it, if possible, because then searching for "SyncRunnableBase" in PSM finds all this kind of badness. My understanding is that you can't use SyncRunnableBase here because SyncRunnableBase would call Notify() immediately after calling RunOnMainThread(), and you need to defer the call to Notify until later; please add a comment to that effect here.

This code is not safe to execute on the socket transport service thread, so we should check that we're not on the socket transport thread. Similarly, I would either move your special "if (NS_IsMainThread()" logic from ::DoOCSPRequest to here (preferable) or check and fail if we're on the main thread here.

From this point forward, we don't need to be holding the monitor, right? IMO it is better for readability to scope "lock" to the minimum scope, even though it doesn't matter for performance here.

@@ +216,5 @@
> +    return nullptr;
> +  }
> +  response->len = dataBuffer.Length();
> +  memcpy(response->data, dataBuffer.BeginReading(), response->len);
> +  return response;

Instead of allocating a new SECItem from the arena, you could have dataBuffer be an output parameter of type nsCString (renamed to encodedOCSPResponse) in both OCSPRequester::DoOCSPrequest and ::DoOCSPRequest. Then you would avoid this copying and you would avoid the less-obvious ownership semantics of the SECItem that is currently being returned.

::: security/certverifier/OCSPRequestor.h
@@ +9,5 @@
> +
> +#include "secmodt.h"
> +
> +namespace mozilla { namespace psm {
> +

Document the ownership and lifetime requirements of the result and of the pointer input parameters. If you take my suggestion of changing the returned SECItem into a nsCString output parameter, you can avoid having the caller create and pass in the arena.

It would be better for DoOCSPrequest to make a copy of url rather than requiring the caller to know it must stay alive. Usually it is good to avoid copying but considering the incredible cost of doing the OCSP request in the first place, the copying is no big deal.
Attachment #8407103 - Flags: review?(brian) → review-
Reporter

Updated

5 years ago
Reporter

Updated

5 years ago
No longer blocks: mozilla::pkix
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #8)
> Note that fetchOcspHttpClientV1 in ocsp.c, which CERT_PostOCSPRequest
> ultimately calls, is a very simply function. If we don't want to do OCSP
> fetching through NSS then we could easily copy/paste/modify
> fetchOcspHttpClientV1 to get the desired semantics (createFcn takes the
> timeout as a parameter) with much less duplication, much less work, and with
> much less chance of something going wrong (e.g. differences in redirect
> behavior caused by the loadgroup changes or the threading changes). Doing
> that would better match the scope of the change we actually need to solve
> this bug.

Sounds good - thanks for looking at that patch anyway.
Posted patch patch v4 (obsolete) — Splinter Review
Attachment #8407103 - Attachment is obsolete: true
Attachment #8413962 - Flags: review?(brian)
Status: NEW → ASSIGNED
Comment on attachment 8413962 [details] [diff] [review]
patch v4

Review of attachment 8413962 [details] [diff] [review]:
-----------------------------------------------------------------

In my review, I suggest changing how the error codes are determined; basically, nsNSSHttpInterface should be the one calling PR_SetError, not OCSPRequester. However, I understand that you just copied that logic from the NSS code and it could be problematic to change that; if you don't want to take those suggestions, that is fine by me, though we should file a follow-up bug to fix it.

Similarly, I suggest using ScopedPtr, but if that turns out to be non-trivial then skip that as well.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +296,5 @@
>    if (!request) {
>      return SECFailure;
>    }
>  
> +  uint32_t timeoutSeconds = 2;

Please convert intervals to PRIntervalTime as early as possible in the code, so we don't have to worry about unit conversion bugs.

@@ +300,5 @@
> +  uint32_t timeoutSeconds = 2;
> +  if (mOCSPFetching == FetchOCSPForEV ||
> +      mOCSPFetching == FetchOCSPForDVHardFail) {
> +    timeoutSeconds = 10;
> +  }

I think it is better to use a switch that asserts in its default case, so that we force ourselves to decide what the timeout should be any time we extend that enumeration.

::: security/certverifier/OCSPRequestor.cpp
@@ +20,5 @@
> +  SEC_HTTP_REQUEST_SESSION pRequestSession = nullptr;
> +  SEC_HTTP_SERVER_SESSION pServerSession = nullptr;
> +  uint16_t myHttpResponseCode;
> +  const char* myHttpResponseData;
> +  uint32_t myHttpResponseDataLen;

Since we're forking this code from NSS we might as well make it match Gecko style. Please move these declarations to closer to where the variables are used/assigned. And, use normal naming (no "p" and "my" prefixes, for example).

@@ +24,5 @@
> +  uint32_t myHttpResponseDataLen;
> +
> +  nsCOMPtr<nsIURLParser> urlParser = do_GetService(NS_STDURLPARSER_CONTRACTID);
> +  if (!urlParser) {
> +    PORT_SetError(SEC_ERROR_NO_MEMORY);

SEC_ERROR_LIBRARY_FAILURE seems better.

Also use PR_SetError in Gecko, not PORT_SetError, which should be used only in NSS. (By convention, according to reviews of my previous patches by wtc.)

@@ +39,5 @@
> +                                    &schemePos, &schemeLen,
> +                                    &authorityPos, &authorityLen,
> +                                    &pathPos, &pathLen);
> +  if (NS_FAILED(rv)) {
> +    PORT_SetError(SEC_ERROR_OCSP_MALFORMED_REQUEST);

PR_SetError(SEC_ERROR_CERT_BAD_ACCESS_LOCATION, 0);

@@ +49,5 @@
> +  rv = urlParser->ParseAuthority(url + authorityPos, authorityLen,
> +                                 nullptr, nullptr, nullptr, nullptr,
> +                                 &hostnamePos, &hostnameLen, &port);
> +  if (NS_FAILED(rv)) {
> +    PORT_SetError(SEC_ERROR_OCSP_MALFORMED_REQUEST);

PR_SetError(SEC_ERROR_CERT_BAD_ACCESS_LOCATION, 0);

@@ +60,5 @@
> +  nsAutoCString hostname(url + authorityPos + hostnamePos, hostnameLen);
> +  if (nsNSSHttpInterface::createSessionFcn(hostname.BeginReading(), port,
> +                                           &pServerSession)
> +        != SECSuccess) {
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);

createSessionFcn is responsible for calling PR_SetError, so you don't have to do it here. nsNSSHttpInterface::createSessionFcn should be patched to call PR_SetError since it fails to do so.

@@ +67,5 @@
> +
> +  nsAutoCString path(url + pathPos, pathLen);
> +  if (nsNSSHttpInterface::createFcn(pServerSession, "http",
> +                                    path.BeginReading(), "POST",
> +                                    PR_TicksPerSecond() * timeoutSeconds,

You should change the type of the timeout parameter to be PRIntervalTime so you don't have to do this. But, also, PR_SecondsToInterval should be used instead for the conversion.

@@ +69,5 @@
> +  if (nsNSSHttpInterface::createFcn(pServerSession, "http",
> +                                    path.BeginReading(), "POST",
> +                                    PR_TicksPerSecond() * timeoutSeconds,
> +                                    &pRequestSession) != SECSuccess) {
> +    nsNSSHttpInterface::freeSessionFcn(pServerSession);

pServerSession should be a ScopedXXX type so you don't have to do this manually each time.

@@ +70,5 @@
> +                                    path.BeginReading(), "POST",
> +                                    PR_TicksPerSecond() * timeoutSeconds,
> +                                    &pRequestSession) != SECSuccess) {
> +    nsNSSHttpInterface::freeSessionFcn(pServerSession);
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);

Same here. nsNSSHttpInterface::createFcn should call PR_SetError so you don't need to.

Also, SEC_ERROR_OCSP_SERVER_ERROR shouldn't be used for errors that occur before we do networking.

@@ +77,5 @@
> +
> +  if (nsNSSHttpInterface::setPostDataFcn(pRequestSession,
> +        reinterpret_cast<char*>(encodedRequest->data), encodedRequest->len,
> +        "application/ocsp-request") != SECSuccess) {
> +    nsNSSHttpInterface::freeFcn(pRequestSession);

pRequestSession should be a scoped type too.

@@ +79,5 @@
> +        reinterpret_cast<char*>(encodedRequest->data), encodedRequest->len,
> +        "application/ocsp-request") != SECSuccess) {
> +    nsNSSHttpInterface::freeFcn(pRequestSession);
> +    nsNSSHttpInterface::freeSessionFcn(pServerSession);
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);

Ditto on error handling here.

@@ +83,5 @@
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);
> +    return nullptr;
> +  }
> +
> +  myHttpResponseDataLen = 64 * 1024;

No need to initialize this variable here, AFAICT. If we need to, please document why and document why this value was chosen.

@@ +91,5 @@
> +                                               &myHttpResponseDataLen)
> +        != SECSuccess) {
> +    nsNSSHttpInterface::freeFcn(pRequestSession);
> +    nsNSSHttpInterface::freeSessionFcn(pServerSession);
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);

Ditto above.

@@ +98,5 @@
> +
> +  if (myHttpResponseCode != 200) {
> +    nsNSSHttpInterface::freeFcn(pRequestSession);
> +    nsNSSHttpInterface::freeSessionFcn(pServerSession);
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);

Ditto above.

@@ +106,5 @@
> +  encodedResponse = SECITEM_AllocItem(arena, nullptr, myHttpResponseDataLen);
> +  if (!encodedResponse) {
> +    nsNSSHttpInterface::freeFcn(pRequestSession);
> +    nsNSSHttpInterface::freeSessionFcn(pServerSession);
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);

Ditto above.

@@ +110,5 @@
> +    PORT_SetError(SEC_ERROR_OCSP_SERVER_ERROR);
> +    return nullptr;
> +  }
> +
> +  PORT_Memcpy(encodedResponse->data, myHttpResponseData, myHttpResponseDataLen);

Just use normal memcpy.

::: security/certverifier/OCSPRequestor.h
@@ +10,5 @@
> +#include "secmodt.h"
> +
> +namespace mozilla { namespace psm {
> +
> +SECItem* DoOCSPRequest(PLArenaPool* arena, const char* url,

Document that the arena will own the result. Change the type of the timeout to be PRIntervalTime so that it is more self-documenting and more consistent with the rest of our code.

::: security/certverifier/moz.build
@@ +16,5 @@
>          'ExtendedValidation.cpp',
>      ]
>  
>  LOCAL_INCLUDES += [
> +    '../manager/ssl/src',

It would be great if we could "hg cp ../manager/ssl/src/nsNSSCallbacks.cpp OCSPRequester.cpp" to move the OCSP callback code to OCSPRequester.cpp. Could be done later too though; if so, please file the follow-up bug for doing it.
Attachment #8413962 - Flags: review?(brian) → review-
Reporter

Updated

5 years ago
Blocks: 871954
Thanks for the review, Brian. I filed bug 1004149 for calling PR_SetError and bug 1004154 for moving the nsNSSHttpInterface parts in nsNSSCallback.cpp to OCSPRequestor.cpp.
Posted patch patch v5 (obsolete) — Splinter Review
Attachment #8413962 - Attachment is obsolete: true
Attachment #8415570 - Flags: review?(brian)
Comment on attachment 8415570 [details] [diff] [review]
patch v5

Review of attachment 8415570 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +147,5 @@
> +    case NSSCertDBTrustDomain::NeverFetchOCSP:
> +    case NSSCertDBTrustDomain::LocalOnlyOCSPForEV:
> +      MOZ_CRASH("we should never see this OCSPFetching type here");
> +    default:
> +      MOZ_CRASH("we're not handling every OCSPFetching type");

I feel like MOZ_CRASH might be a little extreme here. I remember when we did something similar for enumerating cipher suites for telemetry, we ended up causing a crash unnecessarily in the release Firefox due to an issue that didn't really need to result in a crash. In this case, I think we could use PR_NOT_REACHED and use the default (2).

::: security/certverifier/OCSPRequestor.cpp
@@ +69,5 @@
> +  nsAutoCString hostname(url + authorityPos + hostnamePos, hostnameLen);
> +  SEC_HTTP_SERVER_SESSION serverSessionPtr = nullptr;
> +  if (nsNSSHttpInterface::createSessionFcn(hostname.BeginReading(), port,
> +                                           &serverSessionPtr) != SECSuccess) {
> +    return nullptr;

Sorry. I should have been more clear. If we are going to defer having createSessionFcn and friends correctly call PR_SetError then we still need to call PR_SetError in this function when those functions fail, because otherwise the error code will never be set due to the bugs in createSessionFcn and friends. So, please add your PR_SetError calls back to this function.

@@ +79,5 @@
> +  SEC_HTTP_REQUEST_SESSION requestSessionPtr;
> +  if (nsNSSHttpInterface::createFcn(serverSession.get(), "http",
> +                                    path.BeginReading(), "POST",
> +                                    timeout, &requestSessionPtr)
> +        != SECSuccess) {

ditto.

@@ +87,5 @@
> +  ScopedHTTPRequestSession requestSession(
> +    reinterpret_cast<nsNSSHttpRequestSession*>(requestSessionPtr));
> +  if (nsNSSHttpInterface::setPostDataFcn(requestSession.get(),
> +        reinterpret_cast<char*>(encodedRequest->data), encodedRequest->len,
> +        "application/ocsp-request") != SECSuccess) {

ditto.
Attachment #8415570 - Flags: review?(brian) → review+
Posted patch patch v5.1 (obsolete) — Splinter Review
Thanks for the review. Carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=8f6026672e56
Attachment #8415570 - Attachment is obsolete: true
Attachment #8415995 - Flags: review+
Had some different-platform-compilers-not-agreeing issues: https://tbpl.mozilla.org/?tree=Try&rev=935f51051e7c
Posted patch patch v5.1.2Splinter Review
Attachment #8415995 - Attachment is obsolete: true
Attachment #8416208 - Flags: review+
Inbound is closed, so using checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/daee17c14581
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Reporter

Updated

5 years ago
Blocks: 1005142
Should this get uplifted to Firefox 31?
Flags: needinfo?(dkeeler)
I believe so, yes.
Flags: needinfo?(dkeeler)
Comment on attachment 8416208 [details] [diff] [review]
patch v5.1.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): mozilla::pkix
User impact if declined: no EV indicator on slow networks
Testing completed (on m-c, etc.): has tests, has been on m-c for a while
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8416208 - Flags: approval-mozilla-aurora?
Attachment #8416208 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs a branch patch for the Aurora uplift.
Flags: needinfo?(dkeeler)
Sorry - I put the cart before the horse, here. This needs uplift, but it isn't quite ready to go yet.
Flags: needinfo?(dkeeler)
You need to log in before you can comment on or make changes to this bug.