Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 1 | # Mojo "Style" Guide |
| 2 | |
| 3 | Mojo is Chrome's new IPC system and provides lots of useful abstractions. These |
| 4 | abstractions can make it easier to write code that makes interprocess calls, |
| 5 | but can also add significant complexity. Below are some recommendation from |
| 6 | Mojo and IPC reviewers for best practices. |
| 7 | |
Michael Giuffrida | 6820821 | 2019-02-11 09:15:21 | [diff] [blame] | 8 | For questions, concerns, or suggestions, reach out to |
| 9 | [[email protected]](https://groups.google.com/a/chromium.org/forum/#!forum/chromium-mojo). |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 10 | |
| 11 | > For legacy IPC, please see [security tips for IPC][security-tips-for-ipc]. |
| 12 | |
| 13 | [TOC] |
| 14 | |
| 15 | |
| 16 | ## Simplicity |
| 17 | |
| 18 | Strive to write simple interfaces. Minimize the amount of cross-process state |
| 19 | that needs to be maintained in sync. |
| 20 | |
| 21 | **_Good_** |
| 22 | |
| 23 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 24 | interface TeleporterFactory { |
Oksana Zhuravlova | 3fde5ed | 2020-04-11 00:40:55 | [diff] [blame] | 25 | Create(Location start, Location end) => (pending_remote<Teleporter>); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 26 | }; |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 27 | |
| 28 | interface Teleporter { |
| 29 | TeleportGoat(Goat) = (); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 30 | }; |
| 31 | ``` |
| 32 | |
| 33 | **_Bad_** |
| 34 | |
| 35 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 36 | interface Teleporter { |
| 37 | // Bad: comments will need to explicitly call out that both locations need to |
| 38 | // be bound before calling TeleportGoat()! |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 39 | // |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 40 | // In addition, if untrustworthy processes can talk to trustworthy processes, |
| 41 | // the Teleporter implementation will need to also handle the case where the |
| 42 | // Location objects are not yet bound. |
| 43 | SetStart(Location); |
| 44 | SetEnd(Location); |
| 45 | TeleportGoat(Goat) = (); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 46 | }; |
| 47 | ``` |
| 48 | |
| 49 | Similarly, strive to make methods focused. Do not overuse optional types. |
| 50 | |
| 51 | **_Good_** |
| 52 | |
| 53 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 54 | struct TeleporterStats { |
| 55 | AnimalStats animal_stats; |
| 56 | FungiStats fungi_stats; |
| 57 | GoatStats goat_stats; |
| 58 | PlantStats plant_stats; |
| 59 | }; |
| 60 | |
| 61 | interface Teleporter { |
| 62 | TeleportAnimal(Animal) => (); |
| 63 | TeleportFungi(Fungi) => (); |
| 64 | TeleportGoat(Goat) = (); |
| 65 | TeleportPlant(Plant) => (); |
| 66 | |
Roland Bock | 3afe8cc | 2020-11-06 23:14:53 | [diff] [blame] | 67 | // TeleporterStats will be have a value if and only if the call was |
| 68 | // successful. |
| 69 | GetStats() => (TeleporterStats?); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 70 | }; |
| 71 | ``` |
| 72 | |
| 73 | **_Bad_** |
| 74 | |
| 75 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 76 | interface Teleporter { |
| 77 | // The intent of four optional arguments is unclear: can this call teleport |
| 78 | // multiple objects of different types at once, or is the caller only |
| 79 | // supposed to only pass one non-null argument per call? |
| 80 | Teleport(Animal?, Fungi?, Goat?, Plant?) => (); |
| 81 | |
Roland Bock | 3afe8cc | 2020-11-06 23:14:53 | [diff] [blame] | 82 | // Does this return all stats if success is true? Or just the categories that |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 83 | // the teleporter already has stats for? The intent is uncertain, so wrapping |
| 84 | // the disparate values into a result struct would be cleaner. |
| 85 | GetStats() => |
| 86 | (bool success, AnimalStats?, FungiStats?, PlantStats?, FungiStats?); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 87 | }; |
| 88 | ``` |
| 89 | |
| 90 | |
| 91 | ## Documentation |
| 92 | |
| 93 | Mojo structs, interfaces, and methods should all have comments. Make sure the |
| 94 | comments cover the "how" and the "why" of using an interface and its methods, |
| 95 | and not just the "what". Document preconditions, postconditions, and trust: if |
| 96 | an interface is implemented in the browser process and handles requests from |
| 97 | the renderer process, this should be mentioned in the comments. Complex features |
| 98 | should also have an external `README.md` that covers the high-level flow of |
| 99 | information through interfaces and how they interact to implement the feature. |
| 100 | |
| 101 | **_Good_** |
| 102 | |
| 103 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 104 | // Interface for controlling a teleporter. Lives in the browser process, and |
| 105 | // used to implement the Teleportation over Mojo IPC RFC. |
| 106 | interface Teleporter { |
| 107 | // Teleportation helpers for different taxonomic kingdoms. Teleportation is |
| 108 | // not complete until the reply callback is invoked. The caller must NOT |
| 109 | // release the sender side resources until the reply callback runs; releasing |
| 110 | // the resources early will cause splinching. |
| 111 | TeleportAnimal(Animal) => (); |
| 112 | TeleportFungi(Fungi) => (); |
| 113 | // Goats require a specialized teleportation channel distinct from |
| 114 | // TeleportAnimal to ensure goatiness isolation. |
| 115 | TeleportGoat(Goat) => (); |
| 116 | TeleportPlant(Plant) => (); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 117 | |
Roland Bock | 3afe8cc | 2020-11-06 23:14:53 | [diff] [blame] | 118 | // Returns current teleporter stats. On failure (e.g. a teleportation |
| 119 | // operation is currently in progress) a null stats object will be returned. |
| 120 | GetStats() => (TeleporterStats?); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 121 | }; |
| 122 | ``` |
| 123 | |
| 124 | |
| 125 | ## Security |
| 126 | |
| 127 | Policy should be controlled solely by the browser process. "Policy" can mean |
| 128 | any number of things, such as sizes, addresses, permissions, URLs, origins, |
| 129 | etc. In an ideal world: |
| 130 | |
| 131 | 1. Unprivileged process asks for a capability from the privileged process that |
| 132 | owns the resource. |
| 133 | 1. Privileged process applies policy to find an implementation for the |
| 134 | capability. |
| 135 | 1. Unprivileged process performs operations on the capability, constrained in |
| 136 | scope. |
| 137 | |
| 138 | The privileged process must own the capability lifecycle. |
| 139 | |
| 140 | |
| 141 | ### Do not trust less privileged processes |
| 142 | |
| 143 | This is the overriding principle for all guidelines in this section. When |
| 144 | receiving data from a less trusted process, treat the data as if it were |
| 145 | generated by a malicious adversary. Message handlers cannot assume that offsets |
| 146 | are valid, calculations won't overflow, et cetera. |
| 147 | |
| 148 | In general: |
| 149 | |
| 150 | * the browser process is the most privileged process type and therefore, must |
| 151 | be maximally suspicious of its IPC inputs |
| 152 | * the renderer and the ARC++ processes are the least privileged and least |
| 153 | trustworthy process types |
| 154 | * other process types, such as GPU and plugin, fall in between |
| 155 | |
| 156 | When passing objects up a privilege gradient (from less → more privileged), the |
| 157 | callee must validate the inputs before acting on them. When passing objects |
| 158 | down a privilege gradient, such as from browser → renderer, it is OK for the |
| 159 | callee to trust the caller. |
| 160 | |
| 161 | > See also: [Do not Handle Impossible Situations](#Do-not-handle-impossible-situations) |
| 162 | |
| 163 | |
| 164 | ### Do not send unnecessary or privilege-presuming data |
| 165 | |
Oksana Zhuravlova | ee1afd1 | 2020-02-15 00:47:27 | [diff] [blame] | 166 | > Each `BrowserInterfaceBroker` for frames and workers is strongly associated with an |
Darwin Huang | b4bd245 | 2019-10-08 22:56:04 | [diff] [blame] | 167 | > origin. Where possible, prefer to use this associated origin rather than |
| 168 | > sending it over IPC. (See <https://crbug.com/734210> and |
| 169 | > <https://crbug.com/775792/>). |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 170 | |
| 171 | For example, the browser process must not (fully) trust the renderer's claims |
| 172 | about origins. The browser process should already know what origin the renderer |
| 173 | is evaluating, and thus should already have this data (for example, see |
| 174 | `RenderFrameHost::GetLastCommittedOrigin()`). Thus, a method that requires |
| 175 | passing an origin from the renderer to the browser process has a conceptual |
| 176 | error, and quite possibly, a vulnerability. |
| 177 | |
| 178 | > Note: there are currently subtle races when using `GetLastCommittedOrigin()` |
| 179 | > that will be resolved by fixing <https://crbug.com/729021>. |
| 180 | |
| 181 | Similarly, the browser process must not trust the renderer's claims about file |
| 182 | pathnames. It would be unsafe for the browser process to save a downloaded file |
| 183 | to `~/.bashrc` just because the renderer asked. Instead, it would be better for |
| 184 | the browser process to: |
| 185 | |
| 186 | 1. Kill the renderer if `basename(pathname) != pathname`, since the renderer is |
| 187 | obviously compromised if it makes this mistake. |
| 188 | 1. Defang the basename, by removing leading dots, et cetera. Note that the |
| 189 | definition of proper defanging varies per platform. |
| 190 | 1. Prepend its own parent directory to the basename, e.g. ~/Downloads. |
| 191 | |
| 192 | > TODO(https://crbug.com/779196): Even better would be to implement a C++ type |
| 193 | > performs the appropriate sanitizations and recommend its usage directly here. |
| 194 | |
| 195 | |
Lukasz Anforowicz | bb99a856 | 2019-01-25 19:19:38 | [diff] [blame] | 196 | ### Validate privilege-presuming data received over IPC |
| 197 | |
| 198 | If it is not possible to avoid sending privilege-presuming data over IPC (see |
| 199 | the previous section), then such data should be verified before being used. |
| 200 | |
| 201 | * Browser process: |
| 202 | - Use `ChildProcessSecurityPolicy`'s methods like |
| 203 | `CanAccessDataForOrigin` or `CanReadFile` to verify IPC messages |
| 204 | received from less privileged processes. |
| 205 | - When verification fails, ignore the IPC and terminate the renderer process |
| 206 | using `mojo::ReportBadMessage` (or using `mojo::GetBadMessageCallback` for |
| 207 | messages handled asynchronously). For legacy IPC, the renderer process |
| 208 | may be terminated by calling the `ReceivedBadMessage` function (separate |
| 209 | implementations exist for `//content`, `//chrome` and other layers). |
| 210 | |
| 211 | * NetworkService process: |
| 212 | - Do not trust `network::ResourceRequest::request_initiator` - verify it |
| 213 | using `VerifyRequestInitiatorLock` and fall back to a fail-safe origin |
| 214 | (e.g. an opaque origin) when verification fails. |
| 215 | |
| 216 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 217 | ### Do not define unused or unimplemented things |
| 218 | |
| 219 | Mojo interfaces often cross privilege boundaries. Having well-defined interfaces |
| 220 | that don't contain stubbed out methods or unused parameters makes it easier to |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 221 | understand and evaluate the implications of crossing these boundaries. Several |
| 222 | common areas to watch out for: |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 223 | |
| 224 | |
| 225 | #### Do use EnableIf to guard platform-specific constructs |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 226 | |
| 227 | Platform-specific functionality should only be defined on the platforms where |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 228 | it is implemented. Use the Mojo `EnableIf` annotation to guard definitions that |
| 229 | should only be visible in certain build configurations. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 230 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 231 | **_Good_** |
| 232 | ```c++ |
| 233 | // GN file: |
| 234 | mojom("view_bindings") { |
| 235 | // ... |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 236 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 237 | enabled_features = [] |
| 238 | if (is_android) { |
Bill Orr | dc01ca3 | 2019-01-30 00:22:11 | [diff] [blame] | 239 | enabled_features += [ "is_android" ] |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 240 | } |
| 241 | } |
| 242 | |
| 243 | // mojom definition: |
| 244 | interface View { |
| 245 | // ... |
| 246 | |
| 247 | [EnableIf=is_android] |
| 248 | UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 249 | bool animate); |
| 250 | }; |
| 251 | |
| 252 | // C++ implementation: |
| 253 | class View : public mojom::View { |
| 254 | public: |
| 255 | // ... |
| 256 | |
| 257 | #if defined(OS_ANDROID) |
| 258 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 259 | bool animate); |
| 260 | #endif |
| 261 | }; |
| 262 | ``` |
| 263 | |
| 264 | **_Bad_** |
| 265 | ```c++ |
| 266 | // mojom definition: |
| 267 | interface View { |
| 268 | // ... |
| 269 | |
| 270 | UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 271 | bool animate); |
| 272 | }; |
| 273 | |
| 274 | // C++ implementation: |
| 275 | class View : public mojom::View { |
| 276 | public: |
| 277 | // ... |
| 278 | |
| 279 | #if defined(OS_ANDROID) |
| 280 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 281 | bool animate) override; |
| 282 | #else |
| 283 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 284 | bool animate) override { |
| 285 | NOTREACHED(); |
| 286 | } |
| 287 | #endif |
| 288 | }; |
| 289 | ``` |
| 290 | |
Xiaohan Wang | 688f49d | 2018-02-26 19:39:53 | [diff] [blame] | 291 | The `EnableIf` annotation can be applied to almost anything: imports, |
| 292 | interfaces, methods, arguments, constants, structs, struct members, enums, |
| 293 | enumerator values, et cetera. |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 294 | |
| 295 | |
| 296 | #### Do not define unimplemented methods |
| 297 | |
| 298 | Reviewing IPC requires reviewing a concrete implementation of the Mojo |
| 299 | interface, to evaluate how the (possibly untrustworthy) inputs are used, what |
| 300 | outputs are produced, et cetera. If a method is not yet implemented, do not |
| 301 | define it in the interface. |
| 302 | |
| 303 | **_Bad_** |
| 304 | ```c++ |
| 305 | // mojom definition: |
| 306 | interface Spaceship { |
| 307 | EnterHyperspace(); |
| 308 | ExitHyperspace(); |
| 309 | }; |
| 310 | |
| 311 | // C++ implementation: |
| 312 | class SpaceshipPrototype : public mojom::Spaceship { |
| 313 | void EnterHyperspace() { /* TODO(dcheng): Implement. */ } |
| 314 | void ExitHyperspace() { /* TODO(dcheng): Implement. */ } |
| 315 | }; |
| 316 | ``` |
| 317 | |
| 318 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 319 | #### Do not define placeholder enumerator values |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 320 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 321 | Do not define placeholder enumerator values like `kLast`, `kMax`, `kCount`, et |
| 322 | cetera. Instead, rely on the autogenerated `kMaxValue` enumerator emitted for |
| 323 | Mojo C++ bindings. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 324 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 325 | For UMA histograms, logging a Mojo enum is simple: simply use the two argument |
| 326 | version of `UMA_HISTOGRAM_ENUMERATION`: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 327 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 328 | **_Good_** |
| 329 | ```c++ |
| 330 | // mojom definition: |
| 331 | enum GoatStatus { |
| 332 | kHappy, |
| 333 | kSad, |
| 334 | kHungry, |
| 335 | kGoaty, |
| 336 | }; |
| 337 | |
| 338 | // C++: |
| 339 | UMA_HISTOGRAM_ENUMERATION("Goat.Status", status); |
| 340 | ``` |
| 341 | |
| 342 | Using a `kCount` sentinel complicates `switch` statements and makes it harder to |
| 343 | enforce invariants: code needs to actively enforce that the otherwise invalid |
| 344 | `kCount` sentinel value is not incorrectly passed around. |
| 345 | |
| 346 | **_Bad_** |
| 347 | ```c++ |
| 348 | // mojom definition: |
| 349 | enum CatStatus { |
| 350 | kAloof, |
| 351 | kCount, |
| 352 | }; |
| 353 | |
| 354 | // C++ |
| 355 | switch (cat_status) { |
| 356 | case CatStatus::kAloof: |
| 357 | IgnoreHuman(); |
| 358 | break; |
| 359 | case CatStatus::kCount: |
| 360 | // this should never happen |
| 361 | } |
| 362 | ``` |
| 363 | |
| 364 | Defining `kLast` manually results in ugly casts to perform arithmetic: |
| 365 | |
| 366 | **_Bad_** |
| 367 | ```c++ |
| 368 | // mojom definition: |
| 369 | enum WhaleStatus { |
| 370 | kFail, |
| 371 | kNotFail, |
| 372 | kLast = kNotFail, |
| 373 | }; |
| 374 | |
| 375 | // C++: |
| 376 | UMA_HISTOGRAM_ENUMERATION("Whale.Status", status, |
| 377 | static_cast<int>(WhaleStatus::kLast) + 1); |
| 378 | ``` |
| 379 | |
| 380 | For interoperation with legacy IPC, also use `kMaxValue` rather than defining a |
| 381 | custom `kLast`: |
| 382 | |
| 383 | **_Good_** |
| 384 | ```c++ |
| 385 | IPC_ENUM_TRAITS_MAX_VALUE(GoatStatus, GoatStatus::kMaxValue); |
| 386 | ``` |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 387 | |
| 388 | |
| 389 | ### Use structured types |
| 390 | |
| 391 | Where possible, use structured types: this allows the type system to help |
| 392 | enforce that the input data is valid. Common ones to watch out for: |
| 393 | |
Oksana Zhuravlova | c70ec7ef | 2018-03-13 21:08:42 | [diff] [blame] | 394 | * Files: use `mojo_base.mojom.File`, not raw descriptor types like `HANDLE` |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 395 | and `int`. |
Oksana Zhuravlova | c70ec7ef | 2018-03-13 21:08:42 | [diff] [blame] | 396 | * File paths: use `mojo_base.mojom.FilePath`, not `string`. |
Oksana Zhuravlova | 38321d08 | 2018-04-27 23:59:01 | [diff] [blame] | 397 | * JSON: use `mojo_base.mojom.Value`, not `string`. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 398 | * Mojo interfaces: use `Interface` or `Interface&`, not `handle` or |
| 399 | `handle<message_pipe>`. |
Oksana Zhuravlova | 34579e91 | 2018-03-23 00:18:49 | [diff] [blame] | 400 | * Nonces: use `mojo_base.mojom.UnguessableToken`, not `string`. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 401 | * Origins: use `url.mojom.Origin`, not `url.mojom.Url` and certainly not |
| 402 | `string`. |
Oksana Zhuravlova | 57e8dca | 2018-03-20 03:21:34 | [diff] [blame] | 403 | * Time types: use `mojo_base.mojom.TimeDelta` / |
| 404 | `mojo_base.mojom.TimeTicks` / `mojo_base.mojom.Time`, not `int64` / |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 405 | `uint64` / `double` / et cetera. |
| 406 | * URLs: use `url.mojom.Url`, not `string`. |
Daniel Cheng | 876c6632 | 2018-08-29 06:42:31 | [diff] [blame] | 407 | * `array<uint8>` or `string` and `memcpy()`: use a Mojo struct and statically |
| 408 | define the serialized fields. While `memcpy()` may be tempting for its |
| 409 | simplicity, it can leak info in padding. Even worse, `memcpy()` can easily |
| 410 | copy [undocumented fields][serialize-struct-tm-safely] or newly introduced |
| 411 | fields that were never evaluated for safety by the developer or reviewer. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 412 | |
| 413 | **_Good_** |
| 414 | |
| 415 | ```c++ |
| 416 | interface ReportingService { |
Oksana Zhuravlova | 57e8dca | 2018-03-20 03:21:34 | [diff] [blame] | 417 | ReportDeprecation(mojo_base.mojom.TimeTicks time, |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 418 | url.mojom.Url resource, |
| 419 | uint32 line_number); |
| 420 | }; |
| 421 | ``` |
| 422 | |
| 423 | **_Bad_** |
| 424 | |
| 425 | ```c++ |
| 426 | interface ReportingService { |
| 427 | // Bad: unclear what units |time| is or what |data| contains. |
Oksana Zhuravlova | 38321d08 | 2018-04-27 23:59:01 | [diff] [blame] | 428 | ReportDeprecation(double time, mojo_base.mojom.Value data); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 429 | }; |
| 430 | ``` |
| 431 | |
Daniel Cheng | 876c6632 | 2018-08-29 06:42:31 | [diff] [blame] | 432 | Avoid parallel arrays of data that require the receiver to validate that the |
| 433 | arrays have matching lengths. Instead, bundle the data together in a struct so |
| 434 | it is impossible to have a mismatch: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 435 | |
| 436 | **_Good_** |
| 437 | |
| 438 | ```c++ |
| 439 | struct Pixel { |
| 440 | int8 reds; |
| 441 | int8 greens; |
| 442 | int8 blues; |
| 443 | int8 alphas; |
| 444 | }; |
| 445 | |
| 446 | struct Bitmap { |
| 447 | // Good: it is impossible for there to be mismatched data. |
| 448 | array<Pixel> pixels; |
| 449 | }; |
| 450 | ``` |
| 451 | |
| 452 | **_Bad_** |
| 453 | |
| 454 | ```c++ |
| 455 | // Bad: code using this struct will need to validate that all the arrays have |
| 456 | // matching sizes. |
| 457 | struct Bitmap { |
| 458 | array<int8> reds; |
| 459 | array<int8> greens; |
| 460 | array<int8> blues; |
| 461 | array<int8> alphas; |
| 462 | }; |
| 463 | ``` |
| 464 | |
| 465 | |
| 466 | ### Beware of arithmetic overflow |
| 467 | |
| 468 | > TODO(dcheng): Import the guidance from the legacy IPC doc. |
| 469 | |
| 470 | Signed overflow is undefined in C++. If unsure about whether or not something |
| 471 | will overflow, use the safe numeric helpers from `//base/numerics`! |
| 472 | |
| 473 | **_Good_** |
| 474 | |
| 475 | ```c++ |
| 476 | base::CheckedNumeric<int32_t> size = mojo_rect->width(); |
| 477 | size *= mojo_rect.height(); |
| 478 | if (!size.IsValid()) { |
| 479 | mojo::ReportBadMessage("Bad size from renderer!"); |
| 480 | } |
| 481 | ``` |
| 482 | |
| 483 | **_Bad_** |
| 484 | |
| 485 | ```c++ |
| 486 | // Bad: Signed overflow is undefined in C++! |
| 487 | int32_t size = mojo_rect->width() * mojo_rect.height(); |
| 488 | ``` |
| 489 | |
| 490 | Note that even if the types have defined overflow semantics, it is almost always |
| 491 | a good idea to check for overflow. |
| 492 | |
| 493 | **_Good_** |
| 494 | |
| 495 | ```c++ |
| 496 | uint32_t alloc_size; |
| 497 | if (!CheckMul(request->elements(), request->element_size()) |
| 498 | .AssignIfValid(&alloc_size)) { |
| 499 | // Safe: avoids allocating with a bogus size that overflowed to a smaller than |
| 500 | // expected value. |
Rayan Kanso | 81aeee3 | 2019-07-11 18:50:44 | [diff] [blame] | 501 | mojo::ReportBadMessage("Invalid allocation size"); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 502 | } |
| 503 | |
| 504 | Element* array = CreateArray(alloc_size); |
| 505 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 506 | array[i] = PopulateArray(i); |
| 507 | } |
| 508 | ``` |
| 509 | |
| 510 | **_Bad_** |
| 511 | |
| 512 | ```c++ |
| 513 | uint32_t alloc_size = request->elements() * request->element_size(); |
| 514 | |
| 515 | // Dangerous: alloc_size can overflow so that CreateArray allocates too little |
| 516 | // memory. Subsequent assignments will turn into an out-of-bound write! |
| 517 | Element* array = CreateArray(alloc_size); |
| 518 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 519 | array[i] = PopulateArray(i); |
| 520 | } |
| 521 | ``` |
| 522 | |
| 523 | |
Erik Chen | 14a4bad | 2021-04-21 17:53:13 | [diff] [blame^] | 524 | ### All possible message values are semantically valid |
| 525 | |
| 526 | When possible, messages should be defined in such a way that all possible values |
| 527 | are semantically valid. As a corollary, avoid having the value of one field |
| 528 | dictate the validity of other fields. |
| 529 | |
| 530 | **_Good_** |
| 531 | |
| 532 | ```c++ |
| 533 | union CreateTokenResult { |
| 534 | // Implies success. |
| 535 | string token; |
| 536 | |
| 537 | // Implies failure. |
| 538 | string error_message; |
| 539 | }; |
| 540 | |
| 541 | struct TokenManager { |
| 542 | CreateToken() => (CreateTokenResult result); |
| 543 | }; |
| 544 | ``` |
| 545 | |
| 546 | **_Bad_** |
| 547 | ```c++ |
| 548 | struct TokenManager { |
| 549 | // Requires caller to handle edge case where |success| is set to true, but |
| 550 | // |token| is null. |
| 551 | CreateToken() => (bool success, string? token, string? error_message); |
| 552 | |
| 553 | // Requires caller to handle edge case where both |token| and |error_message| |
| 554 | // are set, or both are null. |
| 555 | CreateToken() => (string? token, string? error_message); |
| 556 | }; |
| 557 | ``` |
| 558 | |
| 559 | There are some known exceptions to this rule because mojo does not handle |
| 560 | optional primitives. |
| 561 | |
| 562 | **_Allowed because mojo has no support for optional primitives_** |
| 563 | ```c++ |
| 564 | struct Foo { |
| 565 | int32 x; |
| 566 | bool has_x; // does the value of `x` have meaning? |
| 567 | int32 y; |
| 568 | bool has_y; // does the value of `y` have meaning? |
| 569 | }; |
| 570 | ``` |
| 571 | |
| 572 | Another common case where we tolerate imperfect message semantics is |
| 573 | with weakly typed integer [bitfields](#handling-bitfields). |
| 574 | |
| 575 | ### Handling bitfields |
| 576 | |
| 577 | Mojom has no native support for bitfields. There are two common approaches: a |
| 578 | type-safe struct of bools which is a bit clunky (preferred) and an integer-based |
| 579 | approach (allowed but not preferred). |
| 580 | |
| 581 | **_Type-safe bitfields_** |
| 582 | ```c++ |
| 583 | struct VehicleBits { |
| 584 | bool has_car; |
| 585 | bool has_bicycle; |
| 586 | bool has_boat; |
| 587 | }; |
| 588 | |
| 589 | struct Person { |
| 590 | VehicleBits bits; |
| 591 | }; |
| 592 | ``` |
| 593 | |
| 594 | **_Integer based approach_** |
| 595 | ```c++ |
| 596 | struct Person { |
| 597 | const uint64 kHasCar = 1; |
| 598 | const uint64 kHasBicycle = 2; |
| 599 | const uint64 kHasGoat= 4; |
| 600 | |
| 601 | uint32 vehicle_bitfield; |
| 602 | }; |
| 603 | ``` |
| 604 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 605 | ## C++ Best Practices |
| 606 | |
| 607 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 608 | ### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler sparingly |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 609 | |
| 610 | Mojo provides several convenience helpers to automatically invoke a callback if |
| 611 | the callback has not already been invoked in some other way when the callback is |
| 612 | destroyed, e.g.: |
| 613 | |
| 614 | ```c++ |
| 615 | { |
Dan Sanders | ce2b98e | 2021-02-03 06:46:10 | [diff] [blame] | 616 | base::OnceCallback<int> cb = mojo::WrapCallbackWithDefaultInvokeIfNotRun( |
| 617 | base::BindOnce([](int) { ... }), -1); |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 618 | } // |cb| is automatically invoked with an argument of -1. |
| 619 | ``` |
| 620 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 621 | This can be useful for detecting interface errors: |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 622 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 623 | ```c++ |
| 624 | process->GetMemoryStatistics( |
| 625 | mojo::WrapCallbackWithDefaultInvokeIfNotRun( |
Dan Sanders | ce2b98e | 2021-02-03 06:46:10 | [diff] [blame] | 626 | base::BindOnce(&MemoryProfiler::OnReplyFromRenderer), <failure args>)); |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 627 | // If the remote process dies, &MemoryProfiler::OnReplyFromRenderer will be |
| 628 | // invoked with <failure args> when Mojo drops outstanding callbacks due to |
| 629 | // a connection error on |process|. |
| 630 | ``` |
| 631 | |
| 632 | However, due to limitations of the current implementation, it's difficult to |
| 633 | tell if a callback object has invoke-on-destroy behavior. In general: |
| 634 | |
| 635 | 1. Prefer error connection handlers where possible. |
| 636 | 1. Only use the callback helpers for detecting interface errors. These |
| 637 | callbacks may be invoked during destruction and must carefully consider |
| 638 | receiver object lifetime. For more information, please see the |
| 639 | [Mojo documentation][mojo-doc-process-crashes]. |
| 640 | |
| 641 | > Note that using the callback wrappers in the renderer is often unnecessary. |
| 642 | > Message pipes are typically closed as part of a Document shutting down; since |
| 643 | > many Blink objects already inherit `blink::ContextLifecycleObserver`, it is |
| 644 | > usually more idiomatic to use this signal to perform any needed cleanup work. |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 645 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 646 | ### Use StructTraits |
| 647 | |
| 648 | Creating a typemap and defining a `StructTraits` specialization moves the |
| 649 | complexity of serialization, deserialization, and validation into a central |
| 650 | location. We universally recommend this over defining `TypeConverter` |
| 651 | specializations: when a value fails deserialization, the receiver method will |
| 652 | never even be invoked. As a bonus, it also reduces the number of copies during |
| 653 | serialization and deserialization. 😄 |
| 654 | |
| 655 | **_Good_** |
| 656 | |
| 657 | ```c++ |
Daniel Cheng | 1f38693 | 2018-01-29 19:56:47 | [diff] [blame] | 658 | // In url_gurl_mojom_traits.h: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 659 | template <> |
| 660 | struct StructTraits<url::mojom::UrlDataView, GURL> { |
| 661 | static base::StringPiece url(const GURL& r); |
| 662 | |
| 663 | // If Read() returns false, Mojo will discard the message. |
| 664 | static bool Read(url::mojom::UrlDataView data, GURL* out); |
| 665 | }; |
| 666 | |
Daniel Cheng | 1f38693 | 2018-01-29 19:56:47 | [diff] [blame] | 667 | // In url_gurl_mojom_traits.cc: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 668 | // Note that methods that aren't simple getters should be defined |
| 669 | // out-of-line to avoid code bloat. |
| 670 | base::StringPiece StructTraits<url::mojom::UrlDataView, GURL>::url( |
| 671 | const GURL& r) { |
| 672 | if (r.possibly_invalid_spec().length() > url::kMaxURLChars || |
| 673 | !r.is_valid()) { |
| 674 | return base::StringPiece(); |
| 675 | } |
| 676 | return base::StringPiece(r.possibly_invalid_spec().c_str(), |
| 677 | r.possibly_invalid_spec().length()); |
| 678 | } |
| 679 | |
| 680 | bool StructTraits<url::mojom::UrlDataView, GURL>::Read( |
| 681 | url::mojom::UrlDataView data, GURL* out) { |
| 682 | base::StringPiece url_string; |
| 683 | if (!data.ReadUrl(&url_string)) |
| 684 | return false; |
| 685 | if (url_string.length() > url::kMaxURLChars) |
| 686 | return false; |
| 687 | *out = GURL(url_string); |
| 688 | return !url_string.empty() && out->is_valid(); |
| 689 | } |
| 690 | ``` |
| 691 | |
| 692 | **_Bad_** |
| 693 | |
| 694 | ```c++ |
| 695 | template <> |
| 696 | struct TypeConverter<url::mojom::UrlPtr, GURL> { |
| 697 | // Inefficient: this copies data once off the wire to create a |
| 698 | // url.mojom.Url object, then copies it again to create a GURL. |
| 699 | static GURL Convert(const url::mojom::UrlPtr url) { |
| 700 | if (url.url.is_empty()) return GURL(); |
| 701 | // Not good: no way to signal errors, so any code that converts the |
| 702 | // Mojo struct to a GURL will somehow need to check for errors… |
| 703 | // but it can't even be distinguished from the empty URL case! |
| 704 | if (url.url.size() > url::kMaxURLChars) return GURL(); |
| 705 | return GURL(url.url); |
| 706 | } |
| 707 | }; |
| 708 | ``` |
| 709 | |
| 710 | There are also corresponding `EnumTraits` and `UnionTraits` specializations for |
| 711 | mojo enums and unions respectively. |
| 712 | |
| 713 | |
| 714 | ### StructTraits getters should be simple |
| 715 | |
| 716 | Where possible, `StructTraits` should be returning const references or simple |
| 717 | read-only views of the data. Having to create temporary data structures during |
| 718 | serialization should be rare, and it should be even rarer to mutate the input |
| 719 | argument. |
| 720 | |
| 721 | |
| 722 | ### Out-of-line complex serialization/deserialization logic |
| 723 | |
| 724 | A `StructTraits` specialization is almost always fully specialized. Only define |
| 725 | `StructTraits` methods inline in the header if the method is a simple getter |
| 726 | that returns a reference, pointer, or other simple POD. Define all other |
| 727 | methods out-of-line to avoid code bloat. |
| 728 | |
| 729 | |
| 730 | ### Do not write one-off functions to convert to/from Mojo types |
| 731 | |
| 732 | There are some instances where it is simply not possible to define a |
| 733 | `StructTraits` for type mapping: this commonly occurs with Blink IDL and Oilpan |
| 734 | types. In these instances, add a `TypeConverter` specialization rather than |
| 735 | defining a one-off conversion function. This makes it easier to search for and |
| 736 | audit code that does potentially risky type conversions. |
| 737 | |
| 738 | > The use of `TypeConverter` should be limited as much as possible: ideally, |
| 739 | > only use it in renderers. |
| 740 | |
| 741 | **_Good_** |
| 742 | |
| 743 | ```c++ |
| 744 | template <> |
| 745 | struct TypeConverter<IDLDictionary, mojom::blink::DictionaryPtr> { |
| 746 | static IDLDictionary* Convert(const mojom::blink::DictionaryPtr& in) { |
| 747 | // Note that unlike StructTraits, there is no out-of-band way to signal |
| 748 | // failure. |
| 749 | IDLDictionary* out = new IDLDictionary; |
| 750 | out->int_value = in->int_value; |
| 751 | out->str_value = in->str_value; |
| 752 | return out; |
| 753 | } |
| 754 | }; |
| 755 | ``` |
| 756 | |
| 757 | **_Bad_** |
| 758 | |
| 759 | ```c++ |
| 760 | // Using a custom one-off function makes this hard to discover in security |
| 761 | // audits. |
| 762 | IDLDictionary* FromMojo(const mojom::blink::DictionaryPtr& in) { |
| 763 | IDLDictionary* out = new IDLDictionary; |
| 764 | out->int_value = in->int_value; |
| 765 | out->str_value = in->str_value; |
| 766 | return out; |
| 767 | } |
| 768 | ``` |
| 769 | |
| 770 | |
| 771 | ### Use the proper abstractions |
| 772 | |
Oksana Zhuravlova | 9f3b8ef | 2019-08-26 20:27:40 | [diff] [blame] | 773 | `mojo::ReceiverSet` implies multiple clients may connect. If this actually isn't |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 774 | the case, please do not use it. For example, if an interface can be rebound, |
Oksana Zhuravlova | 9f3b8ef | 2019-08-26 20:27:40 | [diff] [blame] | 775 | then use the singular `mojo::Receiver` and simply `reset()` the existing receiver |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 776 | before reusing it. |
| 777 | |
| 778 | |
Daniel Cheng | f48fd97 | 2017-11-21 19:15:53 | [diff] [blame] | 779 | ### Explicitly reject bad input |
| 780 | |
| 781 | While validation should be done inside `StructTraits` specializations when |
| 782 | possible, there are situations where additional checks, e.g. overflow checks, |
| 783 | are needed outside of `StructTraits` specializations. Use |
| 784 | `mojo::ReportBadMessage()` or `mojo::GetBadMessageCallback()` to reject bad |
| 785 | input in these situations. Under the hood, this may record UMAs, kill the |
| 786 | process sending bad input, et cetera. |
| 787 | |
| 788 | * `mojo::ReportBadMessage()`: use to report bad IPC input while a message is |
| 789 | being dispatched on the stack. |
| 790 | * `mojo::GetBadMessageCallback()`: use to generate a callback to report bad |
| 791 | IPC input. The callback must be generated while a message is being |
| 792 | dispatched on the stack; however, the returned callback may be invoked be |
| 793 | freely invoked in asynchronously posted callbacks. |
| 794 | |
| 795 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 796 | ## Java Best Practices |
| 797 | |
| 798 | Unfortunately, there are no strongly established conventions here. Most code |
| 799 | tends to write manual conversion helpers and throw an exception on conversion |
| 800 | failure. See [NfcTypeConverter.java] as one example of how to write conversion |
| 801 | code. |
| 802 | |
| 803 | |
| 804 | ## General Code Health |
| 805 | |
| 806 | |
| 807 | ### Naming Conventions |
| 808 | |
| 809 | Place mojo types in `<top-level namespace>.mojom`. Directories for Mojo traits |
| 810 | should be named `mojom` (preferable) or `ipc`. Legacy names that are also |
| 811 | encountered are `public/interfaces`, `interfaces`, or just `mojo`. |
| 812 | |
| 813 | `mojom` is preferred for consistency between the directory name and the nested |
| 814 | namespace name. |
| 815 | |
| 816 | |
| 817 | ### Do not handle impossible situations |
| 818 | |
| 819 | Do not clutter the code by handling impossible situations. Omitting it makes |
| 820 | the invariants clear. This takes two different forms: |
| 821 | |
| 822 | * A less trustworthy process can be compromised by an adversary and send |
| 823 | arbitrary data. When processing data from a less trustworthy process, do |
| 824 | not attempt to handle this invalid data: just call |
| 825 | `mojo::ReportBadMessage()`. When invoked in the context of processing an |
| 826 | IPC from the renderer, this will kill the renderer process. |
| 827 | * A more trustworthy process must be trusted, by definition. Do not write |
| 828 | code to handle impossible situations "just in case" there's a bug. For |
| 829 | example, the renderer class `content::RenderFrameImpl` must always be |
| 830 | connected to certain control interfaces in the browser. It does not makes |
| 831 | sense to handle a Mojo connection error and try to reconnect: a connection |
| 832 | error signals that the browser process is in the process of deleting the |
| 833 | frame, and any attempt at reconnecting will never succeed. |
| 834 | |
| 835 | |
| 836 | ### Using mojo enums directly when possible |
| 837 | |
| 838 | `EnumTraits` generally do not add much value: incoming Mojo enum values are |
| 839 | already validated before typemapping, so it is guaranteed that the input value |
| 840 | to `EnumTraits<T>::FromMojom()` is already a valid enum value, so the method |
| 841 | itself is just a bunch of boilerplate to map between two very similarly named, |
| 842 | yet slightly different, enums. |
| 843 | |
| 844 | To avoid this, prefer to use the Mojo enum directly when possible. |
| 845 | |
| 846 | |
| 847 | ### Consider the cost of setting up message pipes |
| 848 | |
| 849 | Message pipes are fairly inexpensive, but they are not free either: it takes 6 |
| 850 | control messages to establish a message pipe. Keep this in mind: if the |
| 851 | interface is used relatively frequently, connecting once and reusing the |
| 852 | interface pointer is probably a good idea. |
| 853 | |
| 854 | |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 855 | ## Ensure An Explicit Grant For WebUI Bindings |
| 856 | |
| 857 | WebUI renderers sometimes need to call special, powerful IPC endpoints in a |
| 858 | privileged process. It is important to enforce the constraint that the |
| 859 | privileged callee previously created and blessed the calling process as a WebUI |
| 860 | process, and not as a (potentially compromised) web renderer or other |
| 861 | low-privilege process. |
| 862 | |
| 863 | * Use the standard pattern for instantiating `MojoWebUIController`. WebUI |
Chris Palmer | d40cda9 | 2017-12-04 21:12:46 | [diff] [blame] | 864 | Mojo interfaces must only be exposed through a `MojoWebUIController` subclass. |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 865 | * If there is external functionality that the WebUI needs, make sure to route |
| 866 | it through the Mojo interfaces implemented by the `MojoWebUIController`, to |
| 867 | avoid circumventing access checks. |
| 868 | |
| 869 | |
| 870 | ## Not-Yet-Shipped Features Should Be Feature-Checked On The Privileged Side |
| 871 | |
| 872 | Sometimes, there will be powerful new features that are not yet turned on by |
| 873 | default, such as behind a flag, Finch trial, or [origin |
| 874 | trial](https://www.chromium.org/blink/origin-trials). It is not safe to check |
| 875 | for the feature's availability on the renderer side (or in another low-privilege |
| 876 | process type). Instead, ensure that the check is done in the process that has |
| 877 | power to actually enact the feature. Otherwise, a compromised renderer could opt |
| 878 | itself in to the feature! If the feature might not yet be fully developed and |
| 879 | safe, vulnerabilities could arise. |
| 880 | |
| 881 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 882 | [security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc |
| 883 | [NfcTypeConverter.java]: https://chromium.googlesource.com/chromium/src/+/e97442ee6e8c4cf6bcf7f5623c6fb2cc8cce92ac/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 884 | [mojo-doc-process-crashes]: https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings#Best-practices-for-dealing-with-process-crashes-and-callbacks |
Lukasz Anforowicz | bb99a856 | 2019-01-25 19:19:38 | [diff] [blame] | 885 | [serialize-struct-tm-safely]: https://chromium-review.googlesource.com/c/chromium/src/+/679441 |