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 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 195 | ### Do not define unused or unimplemented things |
| 196 | |
| 197 | Mojo interfaces often cross privilege boundaries. Having well-defined interfaces |
| 198 | that don't contain stubbed out methods or unused parameters makes it easier to |
| 199 | understand and evaluate the implications of crossing these boundaries. Some |
| 200 | common guidelines to follow are below. |
| 201 | |
| 202 | |
| 203 | #### Do use EnableIf to guard platform-specific constructs |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 204 | |
| 205 | Platform-specific functionality should only be defined on the platforms where |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 206 | it is implemented. Use the Mojo `EnableIf` annotation to guard definitions that |
| 207 | should only be visible in certain build configurations. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 208 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 209 | **_Good_** |
| 210 | ```c++ |
| 211 | // GN file: |
| 212 | mojom("view_bindings") { |
| 213 | // ... |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 214 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 215 | enabled_features = [] |
| 216 | if (is_android) { |
| 217 | enabled_features += [ "is_android" ]; |
| 218 | } |
| 219 | } |
| 220 | |
| 221 | // mojom definition: |
| 222 | interface View { |
| 223 | // ... |
| 224 | |
| 225 | [EnableIf=is_android] |
| 226 | UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 227 | bool animate); |
| 228 | }; |
| 229 | |
| 230 | // C++ implementation: |
| 231 | class View : public mojom::View { |
| 232 | public: |
| 233 | // ... |
| 234 | |
| 235 | #if defined(OS_ANDROID) |
| 236 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 237 | bool animate); |
| 238 | #endif |
| 239 | }; |
| 240 | ``` |
| 241 | |
| 242 | **_Bad_** |
| 243 | ```c++ |
| 244 | // mojom definition: |
| 245 | interface View { |
| 246 | // ... |
| 247 | |
| 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) override; |
| 260 | #else |
| 261 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 262 | bool animate) override { |
| 263 | NOTREACHED(); |
| 264 | } |
| 265 | #endif |
| 266 | }; |
| 267 | ``` |
| 268 | |
Xiaohan Wang | 688f49d | 2018-02-26 19:39:53 | [diff] [blame^] | 269 | The `EnableIf` annotation can be applied to almost anything: imports, |
| 270 | interfaces, methods, arguments, constants, structs, struct members, enums, |
| 271 | enumerator values, et cetera. |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 272 | |
| 273 | |
| 274 | #### Do not define unimplemented methods |
| 275 | |
| 276 | Reviewing IPC requires reviewing a concrete implementation of the Mojo |
| 277 | interface, to evaluate how the (possibly untrustworthy) inputs are used, what |
| 278 | outputs are produced, et cetera. If a method is not yet implemented, do not |
| 279 | define it in the interface. |
| 280 | |
| 281 | **_Bad_** |
| 282 | ```c++ |
| 283 | // mojom definition: |
| 284 | interface Spaceship { |
| 285 | EnterHyperspace(); |
| 286 | ExitHyperspace(); |
| 287 | }; |
| 288 | |
| 289 | // C++ implementation: |
| 290 | class SpaceshipPrototype : public mojom::Spaceship { |
| 291 | void EnterHyperspace() { /* TODO(dcheng): Implement. */ } |
| 292 | void ExitHyperspace() { /* TODO(dcheng): Implement. */ } |
| 293 | }; |
| 294 | ``` |
| 295 | |
| 296 | |
| 297 | #### Do not define unused enumerator values |
| 298 | |
| 299 | Avoid the pattern of defining a `LAST` or `MAX` value. The `LAST` value is |
| 300 | typically used in conjunction with legacy IPC macros to validate enums; this is |
| 301 | not needed with Mojo enums, which are automatically validated. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 302 | |
| 303 | The `MAX` value is typically used as an invalid sentinel value for UMA |
| 304 | histograms: unfortunately, simply defining a `MAX` value in a Mojo enum will |
| 305 | cause Mojo to treat it as valid. This forces all IPC handling to do manual |
| 306 | checks that the semantically invalid `MAX` value isn't accidentally or |
| 307 | maliciously passed around. |
| 308 | |
| 309 | > Improving UMA logging is tracked in <https://crbug.com/742517>. |
| 310 | |
| 311 | |
| 312 | ### Use structured types |
| 313 | |
| 314 | Where possible, use structured types: this allows the type system to help |
| 315 | enforce that the input data is valid. Common ones to watch out for: |
| 316 | |
| 317 | * Files: use `mojo.common.mojom.File`, not raw descriptor types like `HANDLE` |
| 318 | and `int`. |
| 319 | * File paths: use `mojo.common.mojom.FilePath`, not `string`. |
| 320 | * JSON: use `mojo.common.mojom.Value`, not `string`. |
| 321 | * Mojo interfaces: use `Interface` or `Interface&`, not `handle` or |
| 322 | `handle<message_pipe>`. |
| 323 | * Nonces: use `mojo.common.mojom.UnguessableToken`, not `string`. |
| 324 | * Origins: use `url.mojom.Origin`, not `url.mojom.Url` and certainly not |
| 325 | `string`. |
| 326 | * Time types: use `mojo.common.mojom.TimeDelta` / |
| 327 | `mojo.common.mojom.TimeTicks` / `mojo.common.mojom.Time`, not `int64` / |
| 328 | `uint64` / `double` / et cetera. |
| 329 | * URLs: use `url.mojom.Url`, not `string`. |
| 330 | |
| 331 | **_Good_** |
| 332 | |
| 333 | ```c++ |
| 334 | interface ReportingService { |
| 335 | ReportDeprecation(mojo.common.mojom.TimeTicks time, |
| 336 | url.mojom.Url resource, |
| 337 | uint32 line_number); |
| 338 | }; |
| 339 | ``` |
| 340 | |
| 341 | **_Bad_** |
| 342 | |
| 343 | ```c++ |
| 344 | interface ReportingService { |
| 345 | // Bad: unclear what units |time| is or what |data| contains. |
| 346 | ReportDeprecation(double time, mojo.common.mojom.Value data); |
| 347 | }; |
| 348 | ``` |
| 349 | |
| 350 | Another anti-pattern to avoid is parallel arrays of data: this requires the |
| 351 | receiver to validate that all the arrays have the same length. Instead, prefer |
| 352 | to pass the data so that it is impossible to have a mismatch. |
| 353 | |
| 354 | **_Good_** |
| 355 | |
| 356 | ```c++ |
| 357 | struct Pixel { |
| 358 | int8 reds; |
| 359 | int8 greens; |
| 360 | int8 blues; |
| 361 | int8 alphas; |
| 362 | }; |
| 363 | |
| 364 | struct Bitmap { |
| 365 | // Good: it is impossible for there to be mismatched data. |
| 366 | array<Pixel> pixels; |
| 367 | }; |
| 368 | ``` |
| 369 | |
| 370 | **_Bad_** |
| 371 | |
| 372 | ```c++ |
| 373 | // Bad: code using this struct will need to validate that all the arrays have |
| 374 | // matching sizes. |
| 375 | struct Bitmap { |
| 376 | array<int8> reds; |
| 377 | array<int8> greens; |
| 378 | array<int8> blues; |
| 379 | array<int8> alphas; |
| 380 | }; |
| 381 | ``` |
| 382 | |
| 383 | |
| 384 | ### Beware of arithmetic overflow |
| 385 | |
| 386 | > TODO(dcheng): Import the guidance from the legacy IPC doc. |
| 387 | |
| 388 | Signed overflow is undefined in C++. If unsure about whether or not something |
| 389 | will overflow, use the safe numeric helpers from `//base/numerics`! |
| 390 | |
| 391 | **_Good_** |
| 392 | |
| 393 | ```c++ |
| 394 | base::CheckedNumeric<int32_t> size = mojo_rect->width(); |
| 395 | size *= mojo_rect.height(); |
| 396 | if (!size.IsValid()) { |
| 397 | mojo::ReportBadMessage("Bad size from renderer!"); |
| 398 | } |
| 399 | ``` |
| 400 | |
| 401 | **_Bad_** |
| 402 | |
| 403 | ```c++ |
| 404 | // Bad: Signed overflow is undefined in C++! |
| 405 | int32_t size = mojo_rect->width() * mojo_rect.height(); |
| 406 | ``` |
| 407 | |
| 408 | Note that even if the types have defined overflow semantics, it is almost always |
| 409 | a good idea to check for overflow. |
| 410 | |
| 411 | **_Good_** |
| 412 | |
| 413 | ```c++ |
| 414 | uint32_t alloc_size; |
| 415 | if (!CheckMul(request->elements(), request->element_size()) |
| 416 | .AssignIfValid(&alloc_size)) { |
| 417 | // Safe: avoids allocating with a bogus size that overflowed to a smaller than |
| 418 | // expected value. |
| 419 | mojo::ReportBadMessge("Invalid allocation size"); |
| 420 | } |
| 421 | |
| 422 | Element* array = CreateArray(alloc_size); |
| 423 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 424 | array[i] = PopulateArray(i); |
| 425 | } |
| 426 | ``` |
| 427 | |
| 428 | **_Bad_** |
| 429 | |
| 430 | ```c++ |
| 431 | uint32_t alloc_size = request->elements() * request->element_size(); |
| 432 | |
| 433 | // Dangerous: alloc_size can overflow so that CreateArray allocates too little |
| 434 | // memory. Subsequent assignments will turn into an out-of-bound write! |
| 435 | Element* array = CreateArray(alloc_size); |
| 436 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 437 | array[i] = PopulateArray(i); |
| 438 | } |
| 439 | ``` |
| 440 | |
| 441 | |
| 442 | ## C++ Best Practices |
| 443 | |
| 444 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 445 | ### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler sparingly |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 446 | |
| 447 | Mojo provides several convenience helpers to automatically invoke a callback if |
| 448 | the callback has not already been invoked in some other way when the callback is |
| 449 | destroyed, e.g.: |
| 450 | |
| 451 | ```c++ |
| 452 | { |
| 453 | base::Callback<int> cb = mojo::WrapCallbackWithDefaultInvokeIfNotRun( |
| 454 | base::Bind([](int) { ... }), -1); |
| 455 | } // |cb| is automatically invoked with an argument of -1. |
| 456 | ``` |
| 457 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 458 | This can be useful for detecting interface errors: |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 459 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 460 | ```c++ |
| 461 | process->GetMemoryStatistics( |
| 462 | mojo::WrapCallbackWithDefaultInvokeIfNotRun( |
| 463 | base::Bind(&MemoryProfiler::OnReplyFromRenderer), <failure args>)); |
| 464 | // If the remote process dies, &MemoryProfiler::OnReplyFromRenderer will be |
| 465 | // invoked with <failure args> when Mojo drops outstanding callbacks due to |
| 466 | // a connection error on |process|. |
| 467 | ``` |
| 468 | |
| 469 | However, due to limitations of the current implementation, it's difficult to |
| 470 | tell if a callback object has invoke-on-destroy behavior. In general: |
| 471 | |
| 472 | 1. Prefer error connection handlers where possible. |
| 473 | 1. Only use the callback helpers for detecting interface errors. These |
| 474 | callbacks may be invoked during destruction and must carefully consider |
| 475 | receiver object lifetime. For more information, please see the |
| 476 | [Mojo documentation][mojo-doc-process-crashes]. |
| 477 | |
| 478 | > Note that using the callback wrappers in the renderer is often unnecessary. |
| 479 | > Message pipes are typically closed as part of a Document shutting down; since |
| 480 | > many Blink objects already inherit `blink::ContextLifecycleObserver`, it is |
| 481 | > usually more idiomatic to use this signal to perform any needed cleanup work. |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 482 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 483 | ### Use StructTraits |
| 484 | |
| 485 | Creating a typemap and defining a `StructTraits` specialization moves the |
| 486 | complexity of serialization, deserialization, and validation into a central |
| 487 | location. We universally recommend this over defining `TypeConverter` |
| 488 | specializations: when a value fails deserialization, the receiver method will |
| 489 | never even be invoked. As a bonus, it also reduces the number of copies during |
| 490 | serialization and deserialization. 😄 |
| 491 | |
| 492 | **_Good_** |
| 493 | |
| 494 | ```c++ |
Daniel Cheng | 1f38693 | 2018-01-29 19:56:47 | [diff] [blame] | 495 | // In url_gurl_mojom_traits.h: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 496 | template <> |
| 497 | struct StructTraits<url::mojom::UrlDataView, GURL> { |
| 498 | static base::StringPiece url(const GURL& r); |
| 499 | |
| 500 | // If Read() returns false, Mojo will discard the message. |
| 501 | static bool Read(url::mojom::UrlDataView data, GURL* out); |
| 502 | }; |
| 503 | |
Daniel Cheng | 1f38693 | 2018-01-29 19:56:47 | [diff] [blame] | 504 | // In url_gurl_mojom_traits.cc: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 505 | // Note that methods that aren't simple getters should be defined |
| 506 | // out-of-line to avoid code bloat. |
| 507 | base::StringPiece StructTraits<url::mojom::UrlDataView, GURL>::url( |
| 508 | const GURL& r) { |
| 509 | if (r.possibly_invalid_spec().length() > url::kMaxURLChars || |
| 510 | !r.is_valid()) { |
| 511 | return base::StringPiece(); |
| 512 | } |
| 513 | return base::StringPiece(r.possibly_invalid_spec().c_str(), |
| 514 | r.possibly_invalid_spec().length()); |
| 515 | } |
| 516 | |
| 517 | bool StructTraits<url::mojom::UrlDataView, GURL>::Read( |
| 518 | url::mojom::UrlDataView data, GURL* out) { |
| 519 | base::StringPiece url_string; |
| 520 | if (!data.ReadUrl(&url_string)) |
| 521 | return false; |
| 522 | if (url_string.length() > url::kMaxURLChars) |
| 523 | return false; |
| 524 | *out = GURL(url_string); |
| 525 | return !url_string.empty() && out->is_valid(); |
| 526 | } |
| 527 | ``` |
| 528 | |
| 529 | **_Bad_** |
| 530 | |
| 531 | ```c++ |
| 532 | template <> |
| 533 | struct TypeConverter<url::mojom::UrlPtr, GURL> { |
| 534 | // Inefficient: this copies data once off the wire to create a |
| 535 | // url.mojom.Url object, then copies it again to create a GURL. |
| 536 | static GURL Convert(const url::mojom::UrlPtr url) { |
| 537 | if (url.url.is_empty()) return GURL(); |
| 538 | // Not good: no way to signal errors, so any code that converts the |
| 539 | // Mojo struct to a GURL will somehow need to check for errors… |
| 540 | // but it can't even be distinguished from the empty URL case! |
| 541 | if (url.url.size() > url::kMaxURLChars) return GURL(); |
| 542 | return GURL(url.url); |
| 543 | } |
| 544 | }; |
| 545 | ``` |
| 546 | |
| 547 | There are also corresponding `EnumTraits` and `UnionTraits` specializations for |
| 548 | mojo enums and unions respectively. |
| 549 | |
| 550 | |
| 551 | ### StructTraits getters should be simple |
| 552 | |
| 553 | Where possible, `StructTraits` should be returning const references or simple |
| 554 | read-only views of the data. Having to create temporary data structures during |
| 555 | serialization should be rare, and it should be even rarer to mutate the input |
| 556 | argument. |
| 557 | |
| 558 | |
| 559 | ### Out-of-line complex serialization/deserialization logic |
| 560 | |
| 561 | A `StructTraits` specialization is almost always fully specialized. Only define |
| 562 | `StructTraits` methods inline in the header if the method is a simple getter |
| 563 | that returns a reference, pointer, or other simple POD. Define all other |
| 564 | methods out-of-line to avoid code bloat. |
| 565 | |
| 566 | |
| 567 | ### Do not write one-off functions to convert to/from Mojo types |
| 568 | |
| 569 | There are some instances where it is simply not possible to define a |
| 570 | `StructTraits` for type mapping: this commonly occurs with Blink IDL and Oilpan |
| 571 | types. In these instances, add a `TypeConverter` specialization rather than |
| 572 | defining a one-off conversion function. This makes it easier to search for and |
| 573 | audit code that does potentially risky type conversions. |
| 574 | |
| 575 | > The use of `TypeConverter` should be limited as much as possible: ideally, |
| 576 | > only use it in renderers. |
| 577 | |
| 578 | **_Good_** |
| 579 | |
| 580 | ```c++ |
| 581 | template <> |
| 582 | struct TypeConverter<IDLDictionary, mojom::blink::DictionaryPtr> { |
| 583 | static IDLDictionary* Convert(const mojom::blink::DictionaryPtr& in) { |
| 584 | // Note that unlike StructTraits, there is no out-of-band way to signal |
| 585 | // failure. |
| 586 | IDLDictionary* out = new IDLDictionary; |
| 587 | out->int_value = in->int_value; |
| 588 | out->str_value = in->str_value; |
| 589 | return out; |
| 590 | } |
| 591 | }; |
| 592 | ``` |
| 593 | |
| 594 | **_Bad_** |
| 595 | |
| 596 | ```c++ |
| 597 | // Using a custom one-off function makes this hard to discover in security |
| 598 | // audits. |
| 599 | IDLDictionary* FromMojo(const mojom::blink::DictionaryPtr& in) { |
| 600 | IDLDictionary* out = new IDLDictionary; |
| 601 | out->int_value = in->int_value; |
| 602 | out->str_value = in->str_value; |
| 603 | return out; |
| 604 | } |
| 605 | ``` |
| 606 | |
| 607 | |
| 608 | ### Use the proper abstractions |
| 609 | |
| 610 | `mojo::BindingSet` implies multiple clients may connect. If this actually isn't |
| 611 | the case, please do not use it. For example, if an interface can be rebound, |
| 612 | then use the singular `mojo::Binding` and simply `Close()` the existing binding |
| 613 | before reusing it. |
| 614 | |
| 615 | |
Daniel Cheng | f48fd97 | 2017-11-21 19:15:53 | [diff] [blame] | 616 | ### Explicitly reject bad input |
| 617 | |
| 618 | While validation should be done inside `StructTraits` specializations when |
| 619 | possible, there are situations where additional checks, e.g. overflow checks, |
| 620 | are needed outside of `StructTraits` specializations. Use |
| 621 | `mojo::ReportBadMessage()` or `mojo::GetBadMessageCallback()` to reject bad |
| 622 | input in these situations. Under the hood, this may record UMAs, kill the |
| 623 | process sending bad input, et cetera. |
| 624 | |
| 625 | * `mojo::ReportBadMessage()`: use to report bad IPC input while a message is |
| 626 | being dispatched on the stack. |
| 627 | * `mojo::GetBadMessageCallback()`: use to generate a callback to report bad |
| 628 | IPC input. The callback must be generated while a message is being |
| 629 | dispatched on the stack; however, the returned callback may be invoked be |
| 630 | freely invoked in asynchronously posted callbacks. |
| 631 | |
| 632 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 633 | ## Java Best Practices |
| 634 | |
| 635 | Unfortunately, there are no strongly established conventions here. Most code |
| 636 | tends to write manual conversion helpers and throw an exception on conversion |
| 637 | failure. See [NfcTypeConverter.java] as one example of how to write conversion |
| 638 | code. |
| 639 | |
| 640 | |
| 641 | ## General Code Health |
| 642 | |
| 643 | |
| 644 | ### Naming Conventions |
| 645 | |
| 646 | Place mojo types in `<top-level namespace>.mojom`. Directories for Mojo traits |
| 647 | should be named `mojom` (preferable) or `ipc`. Legacy names that are also |
| 648 | encountered are `public/interfaces`, `interfaces`, or just `mojo`. |
| 649 | |
| 650 | `mojom` is preferred for consistency between the directory name and the nested |
| 651 | namespace name. |
| 652 | |
| 653 | |
| 654 | ### Do not handle impossible situations |
| 655 | |
| 656 | Do not clutter the code by handling impossible situations. Omitting it makes |
| 657 | the invariants clear. This takes two different forms: |
| 658 | |
| 659 | * A less trustworthy process can be compromised by an adversary and send |
| 660 | arbitrary data. When processing data from a less trustworthy process, do |
| 661 | not attempt to handle this invalid data: just call |
| 662 | `mojo::ReportBadMessage()`. When invoked in the context of processing an |
| 663 | IPC from the renderer, this will kill the renderer process. |
| 664 | * A more trustworthy process must be trusted, by definition. Do not write |
| 665 | code to handle impossible situations "just in case" there's a bug. For |
| 666 | example, the renderer class `content::RenderFrameImpl` must always be |
| 667 | connected to certain control interfaces in the browser. It does not makes |
| 668 | sense to handle a Mojo connection error and try to reconnect: a connection |
| 669 | error signals that the browser process is in the process of deleting the |
| 670 | frame, and any attempt at reconnecting will never succeed. |
| 671 | |
| 672 | |
| 673 | ### Using mojo enums directly when possible |
| 674 | |
| 675 | `EnumTraits` generally do not add much value: incoming Mojo enum values are |
| 676 | already validated before typemapping, so it is guaranteed that the input value |
| 677 | to `EnumTraits<T>::FromMojom()` is already a valid enum value, so the method |
| 678 | itself is just a bunch of boilerplate to map between two very similarly named, |
| 679 | yet slightly different, enums. |
| 680 | |
| 681 | To avoid this, prefer to use the Mojo enum directly when possible. |
| 682 | |
| 683 | |
| 684 | ### Consider the cost of setting up message pipes |
| 685 | |
| 686 | Message pipes are fairly inexpensive, but they are not free either: it takes 6 |
| 687 | control messages to establish a message pipe. Keep this in mind: if the |
| 688 | interface is used relatively frequently, connecting once and reusing the |
| 689 | interface pointer is probably a good idea. |
| 690 | |
| 691 | |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 692 | ## Ensure An Explicit Grant For WebUI Bindings |
| 693 | |
| 694 | WebUI renderers sometimes need to call special, powerful IPC endpoints in a |
| 695 | privileged process. It is important to enforce the constraint that the |
| 696 | privileged callee previously created and blessed the calling process as a WebUI |
| 697 | process, and not as a (potentially compromised) web renderer or other |
| 698 | low-privilege process. |
| 699 | |
| 700 | * Use the standard pattern for instantiating `MojoWebUIController`. WebUI |
Chris Palmer | d40cda9 | 2017-12-04 21:12:46 | [diff] [blame] | 701 | Mojo interfaces must only be exposed through a `MojoWebUIController` subclass. |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 702 | * If there is external functionality that the WebUI needs, make sure to route |
| 703 | it through the Mojo interfaces implemented by the `MojoWebUIController`, to |
| 704 | avoid circumventing access checks. |
| 705 | |
| 706 | |
| 707 | ## Not-Yet-Shipped Features Should Be Feature-Checked On The Privileged Side |
| 708 | |
| 709 | Sometimes, there will be powerful new features that are not yet turned on by |
| 710 | default, such as behind a flag, Finch trial, or [origin |
| 711 | trial](https://www.chromium.org/blink/origin-trials). It is not safe to check |
| 712 | for the feature's availability on the renderer side (or in another low-privilege |
| 713 | process type). Instead, ensure that the check is done in the process that has |
| 714 | power to actually enact the feature. Otherwise, a compromised renderer could opt |
| 715 | itself in to the feature! If the feature might not yet be fully developed and |
| 716 | safe, vulnerabilities could arise. |
| 717 | |
| 718 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 719 | [security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc |
| 720 | [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] | 721 | [mojo-doc-process-crashes]: https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings#Best-practices-for-dealing-with-process-crashes-and-callbacks |