David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 1 | # BoringSSL Style Guide |
| 2 | |
| 3 | BoringSSL usually follows the |
David Benjamin | ecc2591 | 2015-10-30 16:31:40 -0400 | [diff] [blame] | 4 | [Google C++ style guide](https://google.github.io/styleguide/cppguide.html), |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 5 | The rest of this document describes differences and clarifications on |
| 6 | top of the base guide. |
| 7 | |
| 8 | |
| 9 | ## Legacy code |
| 10 | |
| 11 | As a derivative of OpenSSL, BoringSSL contains a lot of legacy code that |
| 12 | does not follow this style guide. Particularly where public API is |
| 13 | concerned, balance consistency within a module with the benefits of a |
| 14 | given rule. Module-wide deviations on naming should be respected while |
| 15 | integer and return value conventions take precedence over consistency. |
| 16 | |
David Benjamin | 1a88df1 | 2016-06-02 17:14:33 -0400 | [diff] [blame] | 17 | Modules from OpenSSL's legacy ASN.1 and X.509 stack are retained for |
| 18 | compatibility and left largely unmodified. To ease importing patches from |
| 19 | upstream, they match OpenSSL's new indentation style. For Emacs, |
| 20 | `doc/openssl-c-indent.el` from OpenSSL may be helpful in this. |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 21 | |
| 22 | |
| 23 | ## Language |
| 24 | |
| 25 | The majority of the project is in C, so C++-specific rules in the |
| 26 | Google style guide do not apply. Support for C99 features depends on |
| 27 | our target platforms. Typically, Chromium's target MSVC is the most |
| 28 | restrictive. |
| 29 | |
David Benjamin | 0ee3193 | 2016-07-11 19:38:56 -0400 | [diff] [blame] | 30 | Variable declarations in the middle of a function or inside a `for` loop are |
| 31 | allowed and preferred where possible. Note that the common `goto err` cleanup |
| 32 | pattern requires lifting some variable declarations. |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 33 | |
David Benjamin | 3536809 | 2017-08-29 16:55:10 -0400 | [diff] [blame^] | 34 | Comments should be `// C99-style` for consistency with C++. |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 35 | |
David Benjamin | 3536809 | 2017-08-29 16:55:10 -0400 | [diff] [blame^] | 36 | When declaring pointer types, `*` should be placed next to the variable name, |
| 37 | not the type. So |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 38 | |
| 39 | uint8_t *ptr; |
| 40 | |
| 41 | not |
| 42 | |
| 43 | uint8_t* ptr; |
| 44 | |
| 45 | Rather than `malloc()` and `free()`, use the wrappers `OPENSSL_malloc()` |
| 46 | and `OPENSSL_free()`. Use the standard C `assert()` function freely. |
| 47 | |
David Benjamin | 17cf2cb | 2016-12-13 01:07:13 -0500 | [diff] [blame] | 48 | Use the following wrappers, found in `crypto/internal.h` instead of the |
| 49 | corresponding C standard library functions. They behave the same but avoid |
| 50 | confusing undefined behavior. |
| 51 | |
| 52 | * `OPENSSL_memchr` |
| 53 | * `OPENSSL_memcmp` |
| 54 | * `OPENSSL_memcpy` |
| 55 | * `OPENSSL_memmove` |
| 56 | * `OPENSSL_memset` |
| 57 | |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 58 | For new constants, prefer enums when the values are sequential and typed |
| 59 | constants for flags. If adding values to an existing set of `#define`s, |
| 60 | continue with `#define`. |
| 61 | |
| 62 | |
David Benjamin | 3536809 | 2017-08-29 16:55:10 -0400 | [diff] [blame^] | 63 | ## libssl |
| 64 | |
| 65 | libssl was originally written in C but is being incrementally rewritten in |
| 66 | C++11. As of writing, much of the style matches our C conventions rather than |
| 67 | Google C++. Additionally, libssl on Linux currently may not depend on the C++ |
| 68 | runtime. See the C++ utilities in `ssl/internal.h` for replacements for |
| 69 | problematic C++ constructs. The `util/check_imported_libraries.go` script may be |
| 70 | used with a shared library build to check if a new construct is okay. |
| 71 | |
| 72 | If unsure, match surrounding code. Discrepancies between it and Google C++ style |
| 73 | will be fixed over time. |
| 74 | |
| 75 | |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 76 | ## Formatting |
| 77 | |
| 78 | Single-statement blocks are not allowed. All conditions and loops must |
| 79 | use braces: |
| 80 | |
| 81 | if (foo) { |
| 82 | do_something(); |
| 83 | } |
| 84 | |
| 85 | not |
| 86 | |
| 87 | if (foo) |
| 88 | do_something(); |
| 89 | |
| 90 | |
| 91 | ## Integers |
| 92 | |
| 93 | Prefer using explicitly-sized integers where appropriate rather than |
| 94 | generic C ones. For instance, to represent a byte, use `uint8_t`, not |
| 95 | `unsigned char`. Likewise, represent a two-byte field as `uint16_t`, not |
| 96 | `unsigned short`. |
| 97 | |
| 98 | Sizes are represented as `size_t`. |
| 99 | |
| 100 | Within a struct that is retained across the lifetime of an SSL |
| 101 | connection, if bounds of a size are known and it's easy, use a smaller |
| 102 | integer type like `uint8_t`. This is a "free" connection footprint |
| 103 | optimization for servers. Don't make code significantly more complex for |
| 104 | it, and do still check the bounds when passing in and out of the |
| 105 | struct. This narrowing should not propagate to local variables and |
| 106 | function parameters. |
| 107 | |
| 108 | When doing arithmetic, account for overflow conditions. |
| 109 | |
| 110 | Except with platform APIs, do not use `ssize_t`. MSVC lacks it, and |
| 111 | prefer out-of-band error signaling for `size_t` (see Return values). |
| 112 | |
| 113 | |
| 114 | ## Naming |
| 115 | |
| 116 | Follow Google naming conventions in C++ files. In C files, use the |
| 117 | following naming conventions for consistency with existing OpenSSL and C |
| 118 | styles: |
| 119 | |
| 120 | Define structs with typedef named `TYPE_NAME`. The corresponding struct |
| 121 | should be named `struct type_name_st`. |
| 122 | |
| 123 | Name public functions as `MODULE_function_name`, unless the module |
| 124 | already uses a different naming scheme for legacy reasons. The module |
| 125 | name should be a type name if the function is a method of a particular |
| 126 | type. |
| 127 | |
| 128 | Some types are allocated within the library while others are initialized |
| 129 | into a struct allocated by the caller, often on the stack. Name these |
| 130 | functions `TYPE_NAME_new`/`TYPE_NAME_free` and |
| 131 | `TYPE_NAME_init`/`TYPE_NAME_cleanup`, respectively. All `TYPE_NAME_free` |
| 132 | functions must do nothing on `NULL` input. |
| 133 | |
| 134 | If a variable is the length of a pointer value, it has the suffix |
| 135 | `_len`. An output parameter is named `out` or has an `out_` prefix. For |
| 136 | instance, For instance: |
| 137 | |
| 138 | uint8_t *out, |
| 139 | size_t *out_len, |
| 140 | const uint8_t *in, |
| 141 | size_t in_len, |
| 142 | |
| 143 | Name public headers like `include/openssl/evp.h` with header guards like |
| 144 | `OPENSSL_HEADER_EVP_H`. Name internal headers like |
| 145 | `crypto/ec/internal.h` with header guards like |
| 146 | `OPENSSL_HEADER_EC_INTERNAL_H`. |
| 147 | |
| 148 | Name enums like `enum unix_hacker_t`. For instance: |
| 149 | |
| 150 | enum should_free_handshake_buffer_t { |
| 151 | free_handshake_buffer, |
| 152 | dont_free_handshake_buffer, |
| 153 | }; |
| 154 | |
| 155 | |
| 156 | ## Return values |
| 157 | |
| 158 | As even `malloc` may fail in BoringSSL, the vast majority of functions |
| 159 | will have a failure case. Functions should return `int` with one on |
| 160 | success and zero on error. Do not overload the return value to both |
| 161 | signal success/failure and output an integer. For example: |
| 162 | |
| 163 | OPENSSL_EXPORT int CBS_get_u16(CBS *cbs, uint16_t *out); |
| 164 | |
| 165 | If a function needs more than a true/false result code, define an enum |
| 166 | rather than arbitrarily assigning meaning to int values. |
| 167 | |
| 168 | If a function outputs a pointer to an object on success and there are no |
| 169 | other outputs, return the pointer directly and `NULL` on error. |
| 170 | |
| 171 | |
| 172 | ## Parameters |
| 173 | |
| 174 | Where not constrained by legacy code, parameter order should be: |
| 175 | |
| 176 | 1. context parameters |
| 177 | 2. output parameters |
| 178 | 3. input parameters |
| 179 | |
| 180 | For example, |
| 181 | |
| 182 | /* CBB_add_asn sets |*out_contents| to a |CBB| into which the contents of an |
| 183 | * ASN.1 object can be written. The |tag| argument will be used as the tag for |
| 184 | * the object. It returns one on success or zero on error. */ |
David Benjamin | 1db42fb | 2016-08-19 16:08:45 -0400 | [diff] [blame] | 185 | OPENSSL_EXPORT int CBB_add_asn1(CBB *cbb, CBB *out_contents, unsigned tag); |
David Benjamin | 0e3f1d8 | 2015-09-03 11:59:44 -0400 | [diff] [blame] | 186 | |
| 187 | |
| 188 | ## Documentation |
| 189 | |
| 190 | All public symbols must have a documentation comment in their header |
| 191 | file. The style is based on that of Go. The first sentence begins with |
| 192 | the symbol name, optionally prefixed with "A" or "An". Apart from the |
| 193 | initial mention of symbol, references to other symbols or parameter |
| 194 | names should be surrounded by |pipes|. |
| 195 | |
| 196 | Documentation should be concise but completely describe the exposed |
| 197 | behavior of the function. Pay special note to success/failure behaviors |
| 198 | and caller obligations on object lifetimes. If this sacrifices |
| 199 | conciseness, consider simplifying the function's behavior. |
| 200 | |
| 201 | /* EVP_DigestVerifyUpdate appends |len| bytes from |data| to the data which |
| 202 | * will be verified by |EVP_DigestVerifyFinal|. It returns one on success and |
| 203 | * zero otherwise. */ |
| 204 | OPENSSL_EXPORT int EVP_DigestVerifyUpdate(EVP_MD_CTX *ctx, const void *data, |
| 205 | size_t len); |
| 206 | |
| 207 | Explicitly mention any surprising edge cases or deviations from common |
| 208 | return value patterns in legacy functions. |
| 209 | |
| 210 | /* RSA_private_encrypt encrypts |flen| bytes from |from| with the private key in |
| 211 | * |rsa| and writes the encrypted data to |to|. The |to| buffer must have at |
| 212 | * least |RSA_size| bytes of space. It returns the number of bytes written, or |
| 213 | * -1 on error. The |padding| argument must be one of the |RSA_*_PADDING| |
| 214 | * values. If in doubt, |RSA_PKCS1_PADDING| is the most common. |
| 215 | * |
| 216 | * WARNING: this function is dangerous because it breaks the usual return value |
| 217 | * convention. Use |RSA_sign_raw| instead. */ |
| 218 | OPENSSL_EXPORT int RSA_private_encrypt(int flen, const uint8_t *from, |
| 219 | uint8_t *to, RSA *rsa, int padding); |
| 220 | |
| 221 | Document private functions in their `internal.h` header or, if static, |
| 222 | where defined. |
David Benjamin | 00019f2 | 2017-07-01 21:42:06 -0400 | [diff] [blame] | 223 | |
| 224 | |
| 225 | ## Build logic |
| 226 | |
| 227 | BoringSSL is used by many projects with many different build tools. |
| 228 | Reimplementing and maintaining build logic in each downstream build is |
| 229 | cumbersome, so build logic should be avoided where possible. Platform-specific |
| 230 | files should be excluded by wrapping the contents in `#ifdef`s, rather than |
| 231 | computing platform-specific file lists. Generated source files such as perlasm |
| 232 | and `err_data.c` may be used in the standalone CMake build but, for downstream |
| 233 | builds, they should be pre-generated in `generate_build_files.py`. |