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