Why DCHECK() is preferred then CHECK()

[Zijie He]

Hi, Chromium-Devs,

After carefully reading the coding style (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED), I found two conclusions,

1. DCHECK() is preferred.

2. Should not handle DCHECK() failures.

Could anyone explain the reason of these two decisions?

(Personally I think they make the following logic unexpectedly. Meanwhile with a very high possibility, the root cause of the failure will be lost in crashing report.)

        .Hzj_jie

[Daniel Cheng]

My personal understanding:

1. DCHECK() and CHECK() are used to assert that something is always true. DCHECK() is preferred because CHECK() has a runtime cost. If something is always true, why pay the runtime cost for it? There are some exceptions: for example, in Blink, we use CHECK() in selective places to crash instead of potentially exposing the user to a security bug. This is a bit of a judgement call sometimes.
   
2. DCHECK() failures shouldn't be handled because DCHECK()s are used to assert that certain preconditions are true. If the precondition doesn't always hold, then it shouldn't be a DCHECK(). For example, when deserializing an IPC message, we can't assume that the sender wrote the correct data (the sender might be in a less privileged process that's been compromised). Instead, we need to handle this with an explicit if condition.
   

Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJBMYs4Tcp8udKVXuAs3UpMT59SwpmVh8A%3Ds4-OW43RDuTYqYw%40mail.gmail.com.

[Michael Giuffrida]

On Wed, Jun 7, 2017 at 7:07 PM Daniel Cheng <dch...@chromium.org> wrote:

My personal understanding:

1. DCHECK() and CHECK() are used to assert that something is always true. DCHECK() is preferred because CHECK() has a runtime cost. If something is always true, why pay the runtime cost for it?

This is an important point for newcomers. I still sometimes fall into this trap:

// DoThing should always be successful in this case.

DCHECK(obj->DoThingAndReturnIfSuccessful());

 

which is deleted by the preprocesser step on non-debug builds. I understand the logic, but it's confusing that expressions inside CHECK() are always compiled and executed while those inside DCHECK are sometimes discarded.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKpDw53DU2e55-65M_X_PWKDOgwck%2BgUyfPn-TXCd5jdSQ%40mail.gmail.com.

[Zijie He]

Thank you for the replying.

I totally agree if the preconditions are always true, we should not waste runtime cost. For the logic we can control, this is totally fine.

But this may not true for OS APIs, which is my main concern. E.g. If an edge case happens only on a specific hardware configuration, but the logic failed to handle it properly. We may end up just ignore them at all and leave the users in unexpected or unusable states.

        .Hzj_jie

[Peter Kasting]

On Wed, Jun 7, 2017 at 8:54 PM, 'Zijie He' via Chromium-dev <chromi...@chromium.org> wrote:

But this may not true for OS APIs, which is my main concern. E.g. If an edge case happens only on a specific hardware configuration, but the logic failed to handle it properly. We may end up just ignore them at all and leave the users in unexpected or unusable states.

If you have an OS API that can fail in real ways, even if unusual (e.g. disk read errors), the code must handle the failure in some appropriate way.  [D]CHECKing that the API calls succeed would be incorrect unless those calls are actually guaranteed to succeed.

PK

[Sergey Ulanov]

A related case (which I think Zijie is concerned about) is when there are bugs in an OS API implementation. For example a function may succeed, but not set some returned values. It may allocates buffer of incorrect size. Callback may be called whet it's not expected. An unexpected value may be returned. The question is: should we try to handle such bugs in an OS API gracefully? 

In my opinion it makes sense only for specific OS bugs that we know exist in some configurations. Otherwise DCHECK is enough.

 

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFB-ys8HmVRACVRkP2%3D3ox9NU90hzDX3%3Dtzztqw8Lxj9ow%40mail.gmail.com.

[Peter Kasting]

On Wed, Jun 7, 2017 at 10:42 PM, Sergey Ulanov <ser...@chromium.org> wrote:

On Wed, Jun 7, 2017 at 9:29 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Jun 7, 2017 at 8:54 PM, 'Zijie He' via Chromium-dev <chromi...@chromium.org> wrote:

But this may not true for OS APIs, which is my main concern. E.g. If an edge case happens only on a specific hardware configuration, but the logic failed to handle it properly. We may end up just ignore them at all and leave the users in unexpected or unusable states.

If you have an OS API that can fail in real ways, even if unusual (e.g. disk read errors), the code must handle the failure in some appropriate way.  [D]CHECKing that the API calls succeed would be incorrect unless those calls are actually guaranteed to succeed.

A related case (which I think Zijie is concerned about) is when there are bugs in an OS API implementation. For example a function may succeed, but not set some returned values. It may allocates buffer of incorrect size. Callback may be called whet it's not expected. An unexpected value may be returned. The question is: should we try to handle such bugs in an OS API gracefully? 

In my opinion it makes sense only for specific OS bugs that we know exist in some configurations. Otherwise DCHECK is enough.

Don't be paranoid.  If an API is documented to do X, and it seems to do X, you don't need to exhaustively handle !X because it's bitwise-possible.  Expect layers of abstraction to hold until proven otherwise; the OS APIs are a layer of abstraction :)

If you know for a fact than an API is broken and sometimes does !X, then of course that has to be handled.

PK

[Sergey Ulanov]

That's exactly my point. I should have wrote "Otherwise DCHECK is more than enough".

[Zijie He]

Thank you all for the explanations, very clear and helpful.

        .Hzj_jie

[Sky Malice]

Reviving this thread, I'm still confused.

I've recently converted some CHECKs to DCHECKs, following documentation and a few DCHECK email chains, after we had a CHECK causing unnecessary crashes on Stable. However, in code review, it was brought up that there are some scenarios that are going to behave very poorly and inconsistently with just DCHECKs.

  auto iter = vector.find(key);

  [D]CEHCK(iter != vector.end());

  // If iter == vector.end(), then it's pointing to the beggining of someone else's memory, right? Is it really better to continue with undefind behavior, potentailly setting someone else up to read/write to the wrong place?

  DoesUnknownStuff(*iter);

actaul example and a slightly different one

and

void FunctionName(SomeClassName* raw_ptr) {

  [D]CHECK(raw_ptr);

  // If this is not a virtual call, and if it doesn't access a member field, it may not crash, I think... If it is a large object and accesses only high memory members, those reads might succeed, and be from someone else's actual memory. Potentailly being platform dependant how high memory members would need to be.

  raw_ptr->NonVirtualMethod();

}

So I'm left confused and conflicted. In these scenarios, we'd probably maybe crash, eventaully, sometimes with confusing crash stacks with DCHECKs, but CHECKs would make that a lot more consistent and less confusing. Yes, CHECK has a runtime cost we'd have to pay forever. I'm curious what others think, and hoping we can clarify the documentation to make the right choice explicit in these cases.

Or was my initial assumption wrong, and these cases are all actaully potential security vulnerabilities that justify CHECKs?

Thanks,

Sky

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJBMYs4knR0aOFZJXndQiCYVRhUxmKUpOBBW9u6iA1ghYKARag%40mail.gmail.com.

[Marijn Kruisselbrink]

At least your example of an unnecessary crash looks like it should be neither CHECK nor DCHECK. You're dealing with external data, which you can't assume is valid, so you have to properly deal with corruption. When validating data coming from some untrusted source (network, disk, IPC from renderer, etc) DCHECK/CHECK is never the correct choice.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAGLE5Tdk_9g62SiE2fEQ26RgtYaT6BVv%2BB9WkHQmOtREsRszPA%40mail.gmail.com.

[Peter Kasting]

On Tue, Oct 10, 2017 at 3:49 PM, Sky Malice <sk...@chromium.org> wrote:

Reviving this thread, I'm still confused.

I've recently converted some CHECKs to DCHECKs, following documentation and a few DCHECK email chains, after we had a CHECK causing unnecessary crashes on Stable. However, in code review, it was brought up that there are some scenarios that are going to behave very poorly and inconsistently with just DCHECKs.

"Going to behave" reads like a statement that assumes the DCHECKs can conceivably fail.  If that's true these shouldn't be any kind of a CHECK (see below).

The assumption behind (D)CHECK must always be that the assertion is known _never_ to fail, even in exceptional cases*.  It's not a concern what happens if it fails, because it doesn't fail.

*Assuming other code obeys its contracts.  "Exceptional cases" here refers to things like the hard drive running out of space or the network cable getting suddenly yanked; it doesn't refer to your fellow coders writing a method that says it's guaranteed never to return null, and introducing a bug which makes it return null.

 

  auto iter = vector.find(key);

  [D]CEHCK(iter != vector.end());

  // If iter == vector.end(), then it's pointing to the beggining of someone else's memory, right? Is it really better to continue with undefind behavior, potentailly setting someone else up to read/write to the wrong place?

  DoesUnknownStuff(*iter);

actaul example and a slightly different one

and

void FunctionName(SomeClassName* raw_ptr) {

  [D]CHECK(raw_ptr);

  // If this is not a virtual call, and if it doesn't access a member field, it may not crash, I think... If it is a large object and accesses only high memory members, those reads might succeed, and be from someone else's actual memory. Potentailly being platform dependant how high memory members would need to be.

  raw_ptr->NonVirtualMethod();

}

So I'm left confused and conflicted. In these scenarios, we'd probably maybe crash, eventaully, sometimes with confusing crash stacks with DCHECKs, but CHECKs would make that a lot more consistent and less confusing. Yes, CHECK has a runtime cost we'd have to pay forever. I'm curious what others think, and hoping we can clarify the documentation to make the right choice explicit in these cases.

If you know these assertions hold, then these should be DCHECKs.

Don't use CHECK just because something might theoretically be used as part of an exploit (e.g. a pointer deref).  Use it if we know for certain that introducing code that violates the assertion would open a security hole (e.g. that we correctly sanity-checked untrusted data from another process).

DCHECK is a statement of documentation that a particular condition is known to hold in the code at that place.  CHECK is that, plus a temporary debugging aid for crashes that seem to be due to violated assertions, as well as (in very rare cases) a forcible kill switch on code that is particularly sensitive to security issues.  It's not a catchall for any potentially-exploitable coding bug, since in the limit it is not always obvious what is exploitable.

If Marijn's statement is correct and you can't actually guarantee that the assertions hold, then as mentioned, these should not be (D)CHECKs but rather conditional code to handle errors.

PK

[tsnia...@vewd.com]

Does that mean cases like https://cs.chromium.org/chromium/src/net/extras/sqlite/sqlite_persistent_cookie_store.cc?q=sqlite_persistent_cookie_store.cc&sq=package:chromium&dr&l=661 should be reported as bugs and fixed? The code there essentially does "if (database corrupted) { DCHECK(false); reset_database(); }".

There's also some "I think this never happens and want to know if it does, so I put a DCHECK" instances, mainly in Blink from our downstream experience. These are often difficult to talk about as they usually happen randomly, on nonstandard configurations and without a reproduction scenario.

--

TŚ

[Adam Rice]

On 11 October 2017 at 15:55, <tsnia...@vewd.com> wrote:

Does that mean cases like https://cs.chromium.org/chromium/src/net/extras/sqlite/sqlite_persistent_cookie_store.cc?q=sqlite_persistent_cookie_store.cc&sq=package:chromium&dr&l=661 should be reported as bugs and fixed? The code there essentially does "if (database corrupted) { DCHECK(false); reset_database(); }".

This is explicitly covered in the style guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#check_dcheck_and-notreached

"A consequence of this is that you should not handle DCHECK() failures, even if failure would result in a crash. "

I think we probably have higher-priority work than fixing all the existing instances in the code base, but reviewers should take care not to permit new attempts to recover from DCHECK and NOTREACHED to creep in.

[tsnia...@vewd.com]

Oh, sure, but can I assume there's a "patches welcome" for cases that we do encounter downstream and have time to fix?

--

TŚ

 

[Peter Kasting]

Patches to fix things like this are OK, but be careful of what the correct fix is.

In the case of SQLite DB corruption, for example, ask whether we could reach this point with a corrupted database due to a disk failure, crash during previous write, or similar.  If yes, that means the DCHECK is wrong and should be removed.  If all possible DB corruption should have already been fixed by the time we reach here, then the right transformation is to condense everything to DCHECK(!database_corrupted);.

The biggest reason the style guide says DCHECKs should never fail is so people don't write code like the above to begin with, where it's ambiguous whether the original author thought failure was completely impossible here, or just exceptional.  When you don't allow yourself to handle errors, it becomes clearer that DCHECKs should only cover the cases where the errors can never occur.

PK 

[Tomasz Śniatowski]

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=774056 for this specific sqlite db case.

We only notice the cases that we do hit, I'm not scourging the code base for violations. They are thus not impossible and the reasonable solution from our perspective is to make the code always handle the error. In general it's difficult for us downstream to reason about such code beyond that/

--
Tomasz Śniatowski

[Peter Kasting]

On Thu, Oct 12, 2017 at 5:51 AM, Tomasz Śniatowski <tsnia...@vewd.com> wrote:

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=774056 for this specific sqlite db case.

We only notice the cases that we do hit, I'm not scourging the code base for violations. They are thus not impossible and the reasonable solution from our perspective is to make the code always handle the error. In general it's difficult for us downstream to reason about such code beyond that/

Hitting the error still doesn't tell you as much as you'd like to know.  If the original author was correct to add the DCHECK, then hitting the error means there's a bug in code reached before this point, which should have prevented you getting here.  Just handling the error is a bandaid that makes it less likely the other bug will be correctly diagnosed and fixed, and thus may be worse than doing nothing.  Generally, the right thing to do is to either reproduce the bug or otherwise understand how it can occur, then examine the control flow along the way to determine where reality doesn't match our expectations; then fix the bug there.  This is often time-consuming and difficult, and if it's not in your power to do, it's best to leave things alone.

PK