|
| 1 | +--- |
| 2 | +applyTo: |
| 3 | + - "**/*.cpp" |
| 4 | + - "**/*.h" |
| 5 | + - "**/*.hpp" |
| 6 | + - "**/*.cc" |
| 7 | + - "**/*.c" |
| 8 | +--- |
| 9 | + |
| 10 | +# Code review – third-party library error handling |
| 11 | + |
| 12 | +When flagging a third-party error-handling issue during review, cite this file (`.github/instructions/reviewing.instructions.md`) so the author can look up the full guidelines. |
| 13 | + |
| 14 | +When reviewing any C++ change that adds or modifies calls to OpenSSL (or another third-party C library), apply the checks below. The goal is to catch unchecked return values and inconsistent error-handling patterns before they reach production. |
| 15 | + |
| 16 | +## General principles |
| 17 | + |
| 18 | +1. **Every call that can fail must be checked.** If a C function documents a failure return (error code, null pointer, negative value, …), the caller must test for it. Silently discarding the result is always a defect in this codebase. |
| 19 | +2. **Use the project's own helpers.** CCF already provides wrapper macros and RAII types for the most common libraries (see tables below). Prefer those over ad-hoc `if` checks so that error messages stay consistent and nothing is accidentally skipped. |
| 20 | +3. **Consistent style within a function.** If the first half of a function uses `CHECK1()` for every OpenSSL call but the second half silently ignores a return value, flag the inconsistency even if the ignored call "usually succeeds." |
| 21 | +4. **Clean up on every error path.** When RAII wrappers are not used, verify that every early-return or throw after a partial allocation frees the already-acquired resources. |
| 22 | + |
| 23 | +## OpenSSL |
| 24 | + |
| 25 | +CCF wraps OpenSSL with helpers defined in `include/ccf/crypto/openssl/openssl_wrappers.h`. |
| 26 | + |
| 27 | +### Available check macros |
| 28 | + |
| 29 | +| Macro | Use when the OpenSSL function … | |
| 30 | +| ---------------------------- | ----------------------------------------------------------------- | |
| 31 | +| `CHECK1(rc)` | returns **1** on success (most `EVP_*`, `BN_*`, `X509_*` setters) | |
| 32 | +| `CHECKNULL(ptr)` | returns a **pointer** that is null on failure | |
| 33 | +| `CHECKPOSITIVE(val)` | returns a **positive int** on success (e.g. `EVP_PKEY_CTX_set_*`) | |
| 34 | +| `CHECKEQUAL(expect, actual)` | must return an **exact value** | |
| 35 | + |
| 36 | +### Available RAII wrappers (`Unique_*`) |
| 37 | + |
| 38 | +`Unique_EVP_PKEY_CTX`, `Unique_BIO`, `Unique_PKEY`, `Unique_X509`, `Unique_X509_REQ`, `Unique_X509_CRL`, `Unique_SSL_CTX`, `Unique_SSL`, `Unique_BIGNUM`, `Unique_X509_TIME`, and others. These call the correct `*_free()` destructor automatically. |
| 39 | + |
| 40 | +### What to look for |
| 41 | + |
| 42 | +- **Allocations without `CHECKNULL`:** Any direct call to `EVP_PKEY_new()`, `BIO_new()`, `X509_new()`, `EVP_MD_CTX_new()`, `BN_new()`, `SSL_CTX_new()`, `SSL_new()`, or similar that stores the result without passing it through `CHECKNULL()` or an equivalent null check. |
| 43 | +- **`CHECK1` vs `CHECKPOSITIVE` mix-ups:** Some OpenSSL functions (notably `EVP_PKEY_CTX_set_*`) return a positive value on success, not exactly 1. Using `CHECK1` on those calls will incorrectly treat valid return codes > 1 as failures and trigger false-positive error handling. Conversely, `CHECKPOSITIVE` is wrong for functions that return exactly 1 on success. |
| 44 | +- **`BIO_get_mem_ptr` / `BIO_read` ignored:** These return an int indicating success. Verify the return is tested before dereferencing the output pointer. |
| 45 | +- **Missing `ERR_get_error` drain on error paths:** When an OpenSSL failure is caught but the error queue is not drained (or vice-versa), later calls may see stale errors. |
| 46 | +- **Raw `new`/`free` instead of RAII wrappers:** If a `Unique_*` type exists for the object, the review should suggest using it rather than manual `*_free()` calls. |
| 47 | +- **Partial checks:** A sequence of OpenSSL calls where some are wrapped in a check macro and others are not is a red flag. All calls in the sequence should be checked. |
| 48 | + |
| 49 | +### Consult the documentation |
| 50 | + |
| 51 | +OpenSSL documents return values on its man pages (<https://docs.openssl.org/master/man3/>). When reviewing a call you are unfamiliar with, look up the specific function to confirm: |
| 52 | + |
| 53 | +- What value indicates success (1, 0, positive, non-null, …). |
| 54 | +- Whether the function sets the OpenSSL error queue on failure. |
| 55 | +- Whether the caller must free the returned object. |
| 56 | + |
| 57 | +Use this to verify that the correct check macro is used and that the error path is appropriate. |
| 58 | + |
| 59 | +## libcurl |
| 60 | + |
| 61 | +CCF wraps libcurl in `src/http/curl.h`. |
| 62 | + |
| 63 | +| Macro | Use when … | |
| 64 | +| -------------------------------------------- | ----------------------------------- | |
| 65 | +| `CHECK_CURL_EASY(fn, ...)` | calling any `curl_easy_*` function | |
| 66 | +| `CHECK_CURL_EASY_SETOPT(handle, opt, arg)` | calling `curl_easy_setopt` | |
| 67 | +| `CHECK_CURL_EASY_GETINFO(handle, info, arg)` | calling `curl_easy_getinfo` | |
| 68 | +| `CHECK_CURL_MULTI(fn, ...)` | calling any `curl_multi_*` function | |
| 69 | + |
| 70 | +### What to look for |
| 71 | + |
| 72 | +- Direct calls to `curl_easy_setopt`, `curl_easy_perform`, or `curl_multi_*` that do not use the above macros. |
| 73 | +- `curl_easy_init()` or `curl_multi_init()` returns not checked for null. |
| 74 | +- `curl_slist_append()` return not checked for null (it returns null on allocation failure). |
| 75 | + |
| 76 | +## llhttp (HTTP/1.x parser) |
| 77 | + |
| 78 | +Used in `src/http/http_parser.h`. The parser entry point is `llhttp_execute()`; its return must be compared against `HPE_OK` (and, where relevant, `HPE_PAUSED_UPGRADE`). |
| 79 | + |
| 80 | +### What to look for |
| 81 | + |
| 82 | +- Calls to `llhttp_execute()` whose return value is not tested. |
| 83 | +- Missing use of `llhttp_errno_name()` / `llhttp_get_error_reason()` in the error message (makes debugging harder). |
| 84 | +- Callback return values: llhttp callbacks (e.g. `on_message_complete`) that return non-zero indicate a parse error to the library. Ensure these are intentional. |
| 85 | + |
| 86 | +## nghttp2 (HTTP/2) |
| 87 | + |
| 88 | +Used in `src/http/http2_callbacks.h` and `src/http/http2_session.h`. Most `nghttp2_*` functions return 0 on success or a negative error code. |
| 89 | + |
| 90 | +### What to look for |
| 91 | + |
| 92 | +- Calls to `nghttp2_session_*`, `nghttp2_submit_*`, or `nghttp2_hd_*` where the return value is silently discarded. |
| 93 | +- Error messages that print only the raw integer code instead of `nghttp2_strerror(rc)`. |
| 94 | +- `nghttp2_session_send()` / `nghttp2_session_mem_recv()` return values not checked. |
| 95 | + |
| 96 | +## QuickJS |
| 97 | + |
| 98 | +Used in `src/js/`. `JS_*` functions return `JSValue`; errors are indicated by `JS_IsException()`. |
| 99 | + |
| 100 | +### What to look for |
| 101 | + |
| 102 | +- `JS_Call`, `JS_Eval`, `JS_GetPropertyStr`, `JS_NewObject`, etc. whose return value is not passed through `JS_IsException()` (or an equivalent check) before use. |
| 103 | +- Missing `JS_FreeValue()` on values that are no longer needed (leaks in the JS runtime). |
| 104 | + |
| 105 | +## Other third-party libraries |
| 106 | + |
| 107 | +For any other C library call added in a change (e.g. `uv_*` from libuv, zlib, or platform APIs), apply the same discipline: |
| 108 | + |
| 109 | +1. Look up the function's documented return-value contract. |
| 110 | +2. Confirm the call site checks for the failure case. |
| 111 | +3. Confirm the error message includes enough context (function name, error code or string) to be debuggable. |
| 112 | +4. Confirm resources are released on the error path. |
| 113 | + |
| 114 | +## Checklist for reviewers |
| 115 | + |
| 116 | +Use this as a mental checklist when reviewing a diff that touches third-party library calls: |
| 117 | + |
| 118 | +- [ ] Every function that can fail has its return value checked. |
| 119 | +- [ ] The correct check macro/pattern is used (e.g. `CHECK1` vs `CHECKPOSITIVE` for OpenSSL). |
| 120 | +- [ ] All allocations are null-checked, ideally via RAII wrappers. |
| 121 | +- [ ] Error handling is consistent within each function — no unchecked calls mixed with checked ones. |
| 122 | +- [ ] Error messages include the library's own error string (e.g. `error_string(ec)`, `nghttp2_strerror(rc)`). |
| 123 | +- [ ] Resources allocated before the failing call are freed on the error path. |
| 124 | +- [ ] No OpenSSL error-queue state is leaked across unrelated operations. |
0 commit comments