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 | |
Alison Gale | 923a33e | 2024-04-22 23:34:28 | [diff] [blame] | 192 | > TODO(crbug.com/41352236): Even better would be to implement a C++ type |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 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 | |
Lukasz Anforowicz | bb99a856 | 2019-01-25 19:19:38 | [diff] [blame] | 211 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 212 | ### Do not define unused or unimplemented things |
| 213 | |
| 214 | Mojo interfaces often cross privilege boundaries. Having well-defined interfaces |
| 215 | 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] | 216 | understand and evaluate the implications of crossing these boundaries. Several |
| 217 | common areas to watch out for: |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 218 | |
| 219 | |
| 220 | #### Do use EnableIf to guard platform-specific constructs |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 221 | |
| 222 | Platform-specific functionality should only be defined on the platforms where |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 223 | it is implemented. Use the Mojo `EnableIf` annotation to guard definitions that |
| 224 | should only be visible in certain build configurations. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 225 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 226 | **_Good_** |
| 227 | ```c++ |
| 228 | // GN file: |
| 229 | mojom("view_bindings") { |
| 230 | // ... |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 231 | |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 232 | enabled_features = [] |
| 233 | if (is_android) { |
Bill Orr | dc01ca3 | 2019-01-30 00:22:11 | [diff] [blame] | 234 | enabled_features += [ "is_android" ] |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 235 | } |
| 236 | } |
| 237 | |
| 238 | // mojom definition: |
| 239 | interface View { |
| 240 | // ... |
| 241 | |
| 242 | [EnableIf=is_android] |
| 243 | UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 244 | bool animate); |
| 245 | }; |
| 246 | |
| 247 | // C++ implementation: |
| 248 | class View : public mojom::View { |
| 249 | public: |
| 250 | // ... |
| 251 | |
Xiaohan Wang | 0727d88 | 2022-01-21 03:03:43 | [diff] [blame] | 252 | #if BUILDFLAG(IS_ANDROID) |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 253 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 254 | bool animate); |
| 255 | #endif |
| 256 | }; |
| 257 | ``` |
| 258 | |
| 259 | **_Bad_** |
| 260 | ```c++ |
| 261 | // mojom definition: |
| 262 | interface View { |
| 263 | // ... |
| 264 | |
| 265 | UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 266 | bool animate); |
| 267 | }; |
| 268 | |
| 269 | // C++ implementation: |
| 270 | class View : public mojom::View { |
| 271 | public: |
| 272 | // ... |
| 273 | |
Xiaohan Wang | 0727d88 | 2022-01-21 03:03:43 | [diff] [blame] | 274 | #if BUILDFLAG(IS_ANDROID) |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 275 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 276 | bool animate) override; |
| 277 | #else |
| 278 | void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing, |
| 279 | bool animate) override { |
Peter Boström | 89c82708 | 2024-09-20 10:54:38 | [diff] [blame] | 280 | NOTREACHED(); |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 281 | } |
| 282 | #endif |
| 283 | }; |
| 284 | ``` |
| 285 | |
Xiaohan Wang | 688f49d | 2018-02-26 19:39:53 | [diff] [blame] | 286 | The `EnableIf` annotation can be applied to almost anything: imports, |
| 287 | interfaces, methods, arguments, constants, structs, struct members, enums, |
| 288 | enumerator values, et cetera. |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 289 | |
| 290 | |
| 291 | #### Do not define unimplemented methods |
| 292 | |
| 293 | Reviewing IPC requires reviewing a concrete implementation of the Mojo |
| 294 | interface, to evaluate how the (possibly untrustworthy) inputs are used, what |
| 295 | outputs are produced, et cetera. If a method is not yet implemented, do not |
| 296 | define it in the interface. |
| 297 | |
| 298 | **_Bad_** |
| 299 | ```c++ |
| 300 | // mojom definition: |
| 301 | interface Spaceship { |
| 302 | EnterHyperspace(); |
| 303 | ExitHyperspace(); |
| 304 | }; |
| 305 | |
| 306 | // C++ implementation: |
| 307 | class SpaceshipPrototype : public mojom::Spaceship { |
| 308 | void EnterHyperspace() { /* TODO(dcheng): Implement. */ } |
| 309 | void ExitHyperspace() { /* TODO(dcheng): Implement. */ } |
| 310 | }; |
| 311 | ``` |
| 312 | |
| 313 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 314 | #### Do not define placeholder enumerator values |
Daniel Cheng | 0a7a4ec | 2018-02-13 23:30:27 | [diff] [blame] | 315 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 316 | Do not define placeholder enumerator values like `kLast`, `kMax`, `kCount`, et |
| 317 | cetera. Instead, rely on the autogenerated `kMaxValue` enumerator emitted for |
| 318 | Mojo C++ bindings. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 319 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 320 | For UMA histograms, logging a Mojo enum is simple: simply use the two argument |
| 321 | version of `UMA_HISTOGRAM_ENUMERATION`: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 322 | |
Daniel Cheng | cda1df5b | 2018-03-30 21:30:16 | [diff] [blame] | 323 | **_Good_** |
| 324 | ```c++ |
| 325 | // mojom definition: |
| 326 | enum GoatStatus { |
| 327 | kHappy, |
| 328 | kSad, |
| 329 | kHungry, |
| 330 | kGoaty, |
| 331 | }; |
| 332 | |
| 333 | // C++: |
| 334 | UMA_HISTOGRAM_ENUMERATION("Goat.Status", status); |
| 335 | ``` |
| 336 | |
| 337 | Using a `kCount` sentinel complicates `switch` statements and makes it harder to |
| 338 | enforce invariants: code needs to actively enforce that the otherwise invalid |
| 339 | `kCount` sentinel value is not incorrectly passed around. |
| 340 | |
| 341 | **_Bad_** |
| 342 | ```c++ |
| 343 | // mojom definition: |
| 344 | enum CatStatus { |
| 345 | kAloof, |
| 346 | kCount, |
| 347 | }; |
| 348 | |
| 349 | // C++ |
| 350 | switch (cat_status) { |
| 351 | case CatStatus::kAloof: |
| 352 | IgnoreHuman(); |
| 353 | break; |
| 354 | case CatStatus::kCount: |
| 355 | // this should never happen |
| 356 | } |
| 357 | ``` |
| 358 | |
| 359 | Defining `kLast` manually results in ugly casts to perform arithmetic: |
| 360 | |
| 361 | **_Bad_** |
| 362 | ```c++ |
| 363 | // mojom definition: |
| 364 | enum WhaleStatus { |
| 365 | kFail, |
| 366 | kNotFail, |
| 367 | kLast = kNotFail, |
| 368 | }; |
| 369 | |
| 370 | // C++: |
| 371 | UMA_HISTOGRAM_ENUMERATION("Whale.Status", status, |
| 372 | static_cast<int>(WhaleStatus::kLast) + 1); |
| 373 | ``` |
| 374 | |
| 375 | For interoperation with legacy IPC, also use `kMaxValue` rather than defining a |
| 376 | custom `kLast`: |
| 377 | |
| 378 | **_Good_** |
| 379 | ```c++ |
| 380 | IPC_ENUM_TRAITS_MAX_VALUE(GoatStatus, GoatStatus::kMaxValue); |
| 381 | ``` |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 382 | |
| 383 | |
| 384 | ### Use structured types |
| 385 | |
| 386 | Where possible, use structured types: this allows the type system to help |
| 387 | enforce that the input data is valid. Common ones to watch out for: |
| 388 | |
Oksana Zhuravlova | c70ec7ef | 2018-03-13 21:08:42 | [diff] [blame] | 389 | * Files: use `mojo_base.mojom.File`, not raw descriptor types like `HANDLE` |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 390 | and `int`. |
Oksana Zhuravlova | c70ec7ef | 2018-03-13 21:08:42 | [diff] [blame] | 391 | * File paths: use `mojo_base.mojom.FilePath`, not `string`. |
Oksana Zhuravlova | 38321d08 | 2018-04-27 23:59:01 | [diff] [blame] | 392 | * JSON: use `mojo_base.mojom.Value`, not `string`. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 393 | * Mojo interfaces: use `Interface` or `Interface&`, not `handle` or |
| 394 | `handle<message_pipe>`. |
Oksana Zhuravlova | 34579e91 | 2018-03-23 00:18:49 | [diff] [blame] | 395 | * Nonces: use `mojo_base.mojom.UnguessableToken`, not `string`. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 396 | * Origins: use `url.mojom.Origin`, not `url.mojom.Url` and certainly not |
| 397 | `string`. |
Oksana Zhuravlova | 57e8dca | 2018-03-20 03:21:34 | [diff] [blame] | 398 | * Time types: use `mojo_base.mojom.TimeDelta` / |
| 399 | `mojo_base.mojom.TimeTicks` / `mojo_base.mojom.Time`, not `int64` / |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 400 | `uint64` / `double` / et cetera. |
Joe Mason | dd55bf8 | 2024-07-16 16:49:47 | [diff] [blame] | 401 | * In WebUI, use `mojo_base.mojom.JSTime` for times coming from Javascript |
| 402 | Date objects. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 403 | * URLs: use `url.mojom.Url`, not `string`. |
Daniel Cheng | 876c6632 | 2018-08-29 06:42:31 | [diff] [blame] | 404 | * `array<uint8>` or `string` and `memcpy()`: use a Mojo struct and statically |
| 405 | define the serialized fields. While `memcpy()` may be tempting for its |
| 406 | simplicity, it can leak info in padding. Even worse, `memcpy()` can easily |
| 407 | copy [undocumented fields][serialize-struct-tm-safely] or newly introduced |
| 408 | fields that were never evaluated for safety by the developer or reviewer. |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 409 | |
| 410 | **_Good_** |
| 411 | |
| 412 | ```c++ |
| 413 | interface ReportingService { |
Oksana Zhuravlova | 57e8dca | 2018-03-20 03:21:34 | [diff] [blame] | 414 | ReportDeprecation(mojo_base.mojom.TimeTicks time, |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 415 | url.mojom.Url resource, |
| 416 | uint32 line_number); |
| 417 | }; |
| 418 | ``` |
| 419 | |
| 420 | **_Bad_** |
| 421 | |
| 422 | ```c++ |
| 423 | interface ReportingService { |
| 424 | // Bad: unclear what units |time| is or what |data| contains. |
Oksana Zhuravlova | 38321d08 | 2018-04-27 23:59:01 | [diff] [blame] | 425 | ReportDeprecation(double time, mojo_base.mojom.Value data); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 426 | }; |
| 427 | ``` |
| 428 | |
Daniel Cheng | 876c6632 | 2018-08-29 06:42:31 | [diff] [blame] | 429 | Avoid parallel arrays of data that require the receiver to validate that the |
| 430 | arrays have matching lengths. Instead, bundle the data together in a struct so |
| 431 | it is impossible to have a mismatch: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 432 | |
| 433 | **_Good_** |
| 434 | |
| 435 | ```c++ |
| 436 | struct Pixel { |
| 437 | int8 reds; |
| 438 | int8 greens; |
| 439 | int8 blues; |
| 440 | int8 alphas; |
| 441 | }; |
| 442 | |
| 443 | struct Bitmap { |
| 444 | // Good: it is impossible for there to be mismatched data. |
| 445 | array<Pixel> pixels; |
| 446 | }; |
| 447 | ``` |
| 448 | |
| 449 | **_Bad_** |
| 450 | |
| 451 | ```c++ |
| 452 | // Bad: code using this struct will need to validate that all the arrays have |
| 453 | // matching sizes. |
| 454 | struct Bitmap { |
| 455 | array<int8> reds; |
| 456 | array<int8> greens; |
| 457 | array<int8> blues; |
| 458 | array<int8> alphas; |
| 459 | }; |
| 460 | ``` |
| 461 | |
| 462 | |
| 463 | ### Beware of arithmetic overflow |
| 464 | |
| 465 | > TODO(dcheng): Import the guidance from the legacy IPC doc. |
| 466 | |
| 467 | Signed overflow is undefined in C++. If unsure about whether or not something |
| 468 | will overflow, use the safe numeric helpers from `//base/numerics`! |
| 469 | |
| 470 | **_Good_** |
| 471 | |
| 472 | ```c++ |
| 473 | base::CheckedNumeric<int32_t> size = mojo_rect->width(); |
| 474 | size *= mojo_rect.height(); |
| 475 | if (!size.IsValid()) { |
| 476 | mojo::ReportBadMessage("Bad size from renderer!"); |
| 477 | } |
| 478 | ``` |
| 479 | |
| 480 | **_Bad_** |
| 481 | |
| 482 | ```c++ |
| 483 | // Bad: Signed overflow is undefined in C++! |
| 484 | int32_t size = mojo_rect->width() * mojo_rect.height(); |
| 485 | ``` |
| 486 | |
| 487 | Note that even if the types have defined overflow semantics, it is almost always |
| 488 | a good idea to check for overflow. |
| 489 | |
| 490 | **_Good_** |
| 491 | |
| 492 | ```c++ |
| 493 | uint32_t alloc_size; |
| 494 | if (!CheckMul(request->elements(), request->element_size()) |
| 495 | .AssignIfValid(&alloc_size)) { |
| 496 | // Safe: avoids allocating with a bogus size that overflowed to a smaller than |
| 497 | // expected value. |
Rayan Kanso | 81aeee3 | 2019-07-11 18:50:44 | [diff] [blame] | 498 | mojo::ReportBadMessage("Invalid allocation size"); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 499 | } |
| 500 | |
| 501 | Element* array = CreateArray(alloc_size); |
| 502 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 503 | array[i] = PopulateArray(i); |
| 504 | } |
| 505 | ``` |
| 506 | |
| 507 | **_Bad_** |
| 508 | |
| 509 | ```c++ |
| 510 | uint32_t alloc_size = request->elements() * request->element_size(); |
| 511 | |
| 512 | // Dangerous: alloc_size can overflow so that CreateArray allocates too little |
| 513 | // memory. Subsequent assignments will turn into an out-of-bound write! |
| 514 | Element* array = CreateArray(alloc_size); |
| 515 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 516 | array[i] = PopulateArray(i); |
| 517 | } |
| 518 | ``` |
| 519 | |
| 520 | |
Erik Chen | 14a4bad | 2021-04-21 17:53:13 | [diff] [blame] | 521 | ### All possible message values are semantically valid |
| 522 | |
| 523 | When possible, messages should be defined in such a way that all possible values |
| 524 | are semantically valid. As a corollary, avoid having the value of one field |
| 525 | dictate the validity of other fields. |
| 526 | |
| 527 | **_Good_** |
| 528 | |
| 529 | ```c++ |
| 530 | union CreateTokenResult { |
| 531 | // Implies success. |
| 532 | string token; |
| 533 | |
| 534 | // Implies failure. |
| 535 | string error_message; |
| 536 | }; |
| 537 | |
| 538 | struct TokenManager { |
| 539 | CreateToken() => (CreateTokenResult result); |
| 540 | }; |
| 541 | ``` |
| 542 | |
| 543 | **_Bad_** |
| 544 | ```c++ |
| 545 | struct TokenManager { |
| 546 | // Requires caller to handle edge case where |success| is set to true, but |
| 547 | // |token| is null. |
| 548 | CreateToken() => (bool success, string? token, string? error_message); |
| 549 | |
| 550 | // Requires caller to handle edge case where both |token| and |error_message| |
| 551 | // are set, or both are null. |
| 552 | CreateToken() => (string? token, string? error_message); |
| 553 | }; |
| 554 | ``` |
| 555 | |
Kurumi Muto | e3f7ca4 | 2024-01-30 12:29:29 | [diff] [blame] | 556 | A known exception where we tolerate imperfect message semantics is |
Erik Chen | 14a4bad | 2021-04-21 17:53:13 | [diff] [blame] | 557 | with weakly typed integer [bitfields](#handling-bitfields). |
| 558 | |
| 559 | ### Handling bitfields |
| 560 | |
| 561 | Mojom has no native support for bitfields. There are two common approaches: a |
| 562 | type-safe struct of bools which is a bit clunky (preferred) and an integer-based |
| 563 | approach (allowed but not preferred). |
| 564 | |
| 565 | **_Type-safe bitfields_** |
| 566 | ```c++ |
| 567 | struct VehicleBits { |
| 568 | bool has_car; |
| 569 | bool has_bicycle; |
| 570 | bool has_boat; |
| 571 | }; |
| 572 | |
| 573 | struct Person { |
| 574 | VehicleBits bits; |
| 575 | }; |
| 576 | ``` |
| 577 | |
| 578 | **_Integer based approach_** |
| 579 | ```c++ |
| 580 | struct Person { |
| 581 | const uint64 kHasCar = 1; |
| 582 | const uint64 kHasBicycle = 2; |
| 583 | const uint64 kHasGoat= 4; |
| 584 | |
| 585 | uint32 vehicle_bitfield; |
| 586 | }; |
| 587 | ``` |
Charlie Harrison | 675df3f | 2023-05-09 21:43:47 | [diff] [blame] | 588 | In both cases, consider typemapping these mojo types to your preferred C++ construct |
| 589 | (e.g. `base::StrongAlias<...>`, `base::EnumSet<...>`, etc.) to improve downstream |
| 590 | readability and type safety. |
Erik Chen | 14a4bad | 2021-04-21 17:53:13 | [diff] [blame] | 591 | |
Andrew Williams | 304e9cf5 | 2022-01-30 23:57:16 | [diff] [blame] | 592 | ### Avoid object lifetime issues with self-owned receivers |
| 593 | |
| 594 | When creating new |
| 595 | [Mojo services](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/mojo_and_services.md) |
| 596 | in the browser process (exposed to the renderer via `BrowserInterfaceBrokers` in |
| 597 | a host object like `RenderFrameHostImpl`, `DedicatedWorkerHost`, etc.), one |
| 598 | approach is to have the interface implementation be owned by the `Receiver` |
| 599 | using `mojo::MakeSelfOwnedReceiver`. From the |
| 600 | [`mojo::MakeSelfOwnedReceiver` declaration](https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/bindings/self_owned_receiver.h;l=129;drc=643cdf61903e99f27c3d80daee67e217e9d280e0): |
| 601 | ``` |
| 602 | // Binds the lifetime of an interface implementation to the lifetime of the |
| 603 | // Receiver. When the Receiver is disconnected (typically by the remote end |
| 604 | // closing the entangled Remote), the implementation will be deleted. |
| 605 | ``` |
| 606 | Consider such an interface created in `RenderFrameHostImpl`, and consider that |
| 607 | and a corresponding `Remote` was created for this interface and owned by |
| 608 | `RenderFrame`. It may seem logical to think that: |
| 609 | 1. (true) The `Receiver` owns the interface implementation |
| 610 | 2. (true) The lifetime of the `Receiver` is based on the lifetime of the |
| 611 | `Remote` in the renderer |
| 612 | 3. (true) The `Remote` is owned by the `RenderFrame` |
| 613 | 4. (true) The lifetime of the `RenderFrameHostImpl` is based on the lifetime of |
| 614 | the `RenderFrame` |
| 615 | 5. (true) Destroying the `RenderFrame` will cause the `Remote` to be destroyed, |
| 616 | ultimately causing the `Receiver` and the interface implementation to be |
| 617 | destroyed. The `RenderFrameHostImpl` will likely be destroyed at some point |
| 618 | as well. |
| 619 | 6. (false) It's safe to assume that `RenderFrameHostImpl` will outlive the |
| 620 | self-owned `Receiver` and interface implementation |
| 621 | |
| 622 | A |
| 623 | [common](https://microsoftedge.github.io/edgevr/posts/yet-another-uaf/) |
| 624 | mistake based on the last assumption above is to store and use a raw pointer |
| 625 | to the `RenderFrameHostImpl` object in the interface implementation. If the |
| 626 | `Receiver` outlives the `RenderFrameHostImpl` and uses the pointer to it, a |
| 627 | Use-After-Free will occur. One way a malicious site or compromised renderer |
| 628 | could make this happen is to generate lots of messages to the interface and then |
| 629 | close the frame. The `Receiver` might have a backlog of messages to process |
| 630 | before it gets the message indicating that the renderer's `Remote` was closed, |
| 631 | and the `RenderFrameHostImpl` can be destroyed in the meantime. |
| 632 | |
| 633 | Similarly, it's not safe to assume that the `Profile` object (and objects owned |
| 634 | by it; `StoragePartitionImpl`, for instance) will outlive the `Receiver`. This |
| 635 | has been observed to be true for at least incognito windows, where a renderer |
| 636 | can generate messages, close the page, and cause the entire window to close |
| 637 | (assuming no other pages are open), ultimately causing the |
| 638 | `OffTheRecordProfileImpl` object to be destroyed before the `Receiver` object. |
| 639 | |
| 640 | To avoid these types of issues, some solutions include: |
| 641 | |
| 642 | - Using `DocumentService` or `DocumentUserData` instead of |
| 643 | `mojo::MakeSelfOwnedReceiver` for document-based interfaces where the |
| 644 | interface implementation needs access to a `RenderFrameHostImpl` object. See |
| 645 | the |
| 646 | [`DocumentService` declaration](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/document_service.h;l=27;drc=d4bf612a0258dd80cfc6d17d49419dd878ebaeb0) |
| 647 | for more details. |
| 648 | |
| 649 | - Having the `Receiver` and/or interface implementation be owned by the object |
| 650 | it relies on (for instance, store the `Receiver` in a private member or |
| 651 | use a `mojo::UniqueReceiverSet` for storing multiple `Receiver` / |
| 652 | interface implementation pairs). |
| 653 | |
| 654 | - Using `WeakPtr`s instead of raw pointers to provide a way to check whether |
| 655 | an object has been destroyed. |
| 656 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 657 | ## C++ Best Practices |
| 658 | |
| 659 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 660 | ### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler sparingly |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 661 | |
| 662 | Mojo provides several convenience helpers to automatically invoke a callback if |
| 663 | the callback has not already been invoked in some other way when the callback is |
| 664 | destroyed, e.g.: |
| 665 | |
| 666 | ```c++ |
| 667 | { |
Dan Sanders | ce2b98e | 2021-02-03 06:46:10 | [diff] [blame] | 668 | base::OnceCallback<int> cb = mojo::WrapCallbackWithDefaultInvokeIfNotRun( |
| 669 | base::BindOnce([](int) { ... }), -1); |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 670 | } // |cb| is automatically invoked with an argument of -1. |
| 671 | ``` |
| 672 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 673 | This can be useful for detecting interface errors: |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 674 | |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 675 | ```c++ |
| 676 | process->GetMemoryStatistics( |
| 677 | mojo::WrapCallbackWithDefaultInvokeIfNotRun( |
Dan Sanders | ce2b98e | 2021-02-03 06:46:10 | [diff] [blame] | 678 | base::BindOnce(&MemoryProfiler::OnReplyFromRenderer), <failure args>)); |
Daniel Cheng | d8df8200 | 2018-02-09 20:52:15 | [diff] [blame] | 679 | // If the remote process dies, &MemoryProfiler::OnReplyFromRenderer will be |
| 680 | // invoked with <failure args> when Mojo drops outstanding callbacks due to |
| 681 | // a connection error on |process|. |
| 682 | ``` |
| 683 | |
| 684 | However, due to limitations of the current implementation, it's difficult to |
| 685 | tell if a callback object has invoke-on-destroy behavior. In general: |
| 686 | |
| 687 | 1. Prefer error connection handlers where possible. |
| 688 | 1. Only use the callback helpers for detecting interface errors. These |
| 689 | callbacks may be invoked during destruction and must carefully consider |
| 690 | receiver object lifetime. For more information, please see the |
| 691 | [Mojo documentation][mojo-doc-process-crashes]. |
| 692 | |
| 693 | > Note that using the callback wrappers in the renderer is often unnecessary. |
| 694 | > Message pipes are typically closed as part of a Document shutting down; since |
| 695 | > many Blink objects already inherit `blink::ContextLifecycleObserver`, it is |
| 696 | > usually more idiomatic to use this signal to perform any needed cleanup work. |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame] | 697 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 698 | ### Use StructTraits |
| 699 | |
| 700 | Creating a typemap and defining a `StructTraits` specialization moves the |
| 701 | complexity of serialization, deserialization, and validation into a central |
| 702 | location. We universally recommend this over defining `TypeConverter` |
| 703 | specializations: when a value fails deserialization, the receiver method will |
| 704 | never even be invoked. As a bonus, it also reduces the number of copies during |
| 705 | serialization and deserialization. 😄 |
| 706 | |
| 707 | **_Good_** |
| 708 | |
| 709 | ```c++ |
Daniel Cheng | 1f38693 | 2018-01-29 19:56:47 | [diff] [blame] | 710 | // In url_gurl_mojom_traits.h: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 711 | template <> |
| 712 | struct StructTraits<url::mojom::UrlDataView, GURL> { |
| 713 | static base::StringPiece url(const GURL& r); |
| 714 | |
| 715 | // If Read() returns false, Mojo will discard the message. |
| 716 | static bool Read(url::mojom::UrlDataView data, GURL* out); |
| 717 | }; |
| 718 | |
Daniel Cheng | 1f38693 | 2018-01-29 19:56:47 | [diff] [blame] | 719 | // In url_gurl_mojom_traits.cc: |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 720 | // Note that methods that aren't simple getters should be defined |
| 721 | // out-of-line to avoid code bloat. |
| 722 | base::StringPiece StructTraits<url::mojom::UrlDataView, GURL>::url( |
| 723 | const GURL& r) { |
| 724 | if (r.possibly_invalid_spec().length() > url::kMaxURLChars || |
| 725 | !r.is_valid()) { |
| 726 | return base::StringPiece(); |
| 727 | } |
| 728 | return base::StringPiece(r.possibly_invalid_spec().c_str(), |
| 729 | r.possibly_invalid_spec().length()); |
| 730 | } |
| 731 | |
| 732 | bool StructTraits<url::mojom::UrlDataView, GURL>::Read( |
| 733 | url::mojom::UrlDataView data, GURL* out) { |
| 734 | base::StringPiece url_string; |
| 735 | if (!data.ReadUrl(&url_string)) |
| 736 | return false; |
| 737 | if (url_string.length() > url::kMaxURLChars) |
| 738 | return false; |
| 739 | *out = GURL(url_string); |
Dominic Farolino | 1f92f2a | 2021-05-27 21:22:41 | [diff] [blame] | 740 | if (!url_string.empty() && !out->is_valid()) |
| 741 | return false; |
| 742 | return true; |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 743 | } |
| 744 | ``` |
| 745 | |
| 746 | **_Bad_** |
| 747 | |
| 748 | ```c++ |
| 749 | template <> |
| 750 | struct TypeConverter<url::mojom::UrlPtr, GURL> { |
| 751 | // Inefficient: this copies data once off the wire to create a |
| 752 | // url.mojom.Url object, then copies it again to create a GURL. |
| 753 | static GURL Convert(const url::mojom::UrlPtr url) { |
| 754 | if (url.url.is_empty()) return GURL(); |
| 755 | // Not good: no way to signal errors, so any code that converts the |
| 756 | // Mojo struct to a GURL will somehow need to check for errors… |
| 757 | // but it can't even be distinguished from the empty URL case! |
| 758 | if (url.url.size() > url::kMaxURLChars) return GURL(); |
| 759 | return GURL(url.url); |
| 760 | } |
| 761 | }; |
| 762 | ``` |
| 763 | |
| 764 | There are also corresponding `EnumTraits` and `UnionTraits` specializations for |
| 765 | mojo enums and unions respectively. |
| 766 | |
| 767 | |
| 768 | ### StructTraits getters should be simple |
| 769 | |
| 770 | Where possible, `StructTraits` should be returning const references or simple |
| 771 | read-only views of the data. Having to create temporary data structures during |
| 772 | serialization should be rare, and it should be even rarer to mutate the input |
| 773 | argument. |
| 774 | |
| 775 | |
| 776 | ### Out-of-line complex serialization/deserialization logic |
| 777 | |
| 778 | A `StructTraits` specialization is almost always fully specialized. Only define |
| 779 | `StructTraits` methods inline in the header if the method is a simple getter |
| 780 | that returns a reference, pointer, or other simple POD. Define all other |
| 781 | methods out-of-line to avoid code bloat. |
| 782 | |
| 783 | |
| 784 | ### Do not write one-off functions to convert to/from Mojo types |
| 785 | |
| 786 | There are some instances where it is simply not possible to define a |
| 787 | `StructTraits` for type mapping: this commonly occurs with Blink IDL and Oilpan |
| 788 | types. In these instances, add a `TypeConverter` specialization rather than |
| 789 | defining a one-off conversion function. This makes it easier to search for and |
| 790 | audit code that does potentially risky type conversions. |
| 791 | |
| 792 | > The use of `TypeConverter` should be limited as much as possible: ideally, |
| 793 | > only use it in renderers. |
| 794 | |
| 795 | **_Good_** |
| 796 | |
| 797 | ```c++ |
| 798 | template <> |
| 799 | struct TypeConverter<IDLDictionary, mojom::blink::DictionaryPtr> { |
| 800 | static IDLDictionary* Convert(const mojom::blink::DictionaryPtr& in) { |
| 801 | // Note that unlike StructTraits, there is no out-of-band way to signal |
| 802 | // failure. |
| 803 | IDLDictionary* out = new IDLDictionary; |
| 804 | out->int_value = in->int_value; |
| 805 | out->str_value = in->str_value; |
| 806 | return out; |
| 807 | } |
| 808 | }; |
| 809 | ``` |
| 810 | |
| 811 | **_Bad_** |
| 812 | |
| 813 | ```c++ |
| 814 | // Using a custom one-off function makes this hard to discover in security |
| 815 | // audits. |
| 816 | IDLDictionary* FromMojo(const mojom::blink::DictionaryPtr& in) { |
| 817 | IDLDictionary* out = new IDLDictionary; |
| 818 | out->int_value = in->int_value; |
| 819 | out->str_value = in->str_value; |
| 820 | return out; |
| 821 | } |
| 822 | ``` |
| 823 | |
| 824 | |
| 825 | ### Use the proper abstractions |
| 826 | |
Oksana Zhuravlova | 9f3b8ef | 2019-08-26 20:27:40 | [diff] [blame] | 827 | `mojo::ReceiverSet` implies multiple clients may connect. If this actually isn't |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 828 | 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] | 829 | then use the singular `mojo::Receiver` and simply `reset()` the existing receiver |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 830 | before reusing it. |
| 831 | |
| 832 | |
Daniel Cheng | f48fd97 | 2017-11-21 19:15:53 | [diff] [blame] | 833 | ### Explicitly reject bad input |
| 834 | |
| 835 | While validation should be done inside `StructTraits` specializations when |
| 836 | possible, there are situations where additional checks, e.g. overflow checks, |
| 837 | are needed outside of `StructTraits` specializations. Use |
| 838 | `mojo::ReportBadMessage()` or `mojo::GetBadMessageCallback()` to reject bad |
| 839 | input in these situations. Under the hood, this may record UMAs, kill the |
| 840 | process sending bad input, et cetera. |
| 841 | |
| 842 | * `mojo::ReportBadMessage()`: use to report bad IPC input while a message is |
| 843 | being dispatched on the stack. |
| 844 | * `mojo::GetBadMessageCallback()`: use to generate a callback to report bad |
| 845 | IPC input. The callback must be generated while a message is being |
| 846 | dispatched on the stack; however, the returned callback may be invoked be |
| 847 | freely invoked in asynchronously posted callbacks. |
| 848 | |
| 849 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 850 | ## Java Best Practices |
| 851 | |
| 852 | Unfortunately, there are no strongly established conventions here. Most code |
| 853 | tends to write manual conversion helpers and throw an exception on conversion |
| 854 | failure. See [NfcTypeConverter.java] as one example of how to write conversion |
| 855 | code. |
| 856 | |
| 857 | |
| 858 | ## General Code Health |
| 859 | |
| 860 | |
| 861 | ### Naming Conventions |
| 862 | |
| 863 | Place mojo types in `<top-level namespace>.mojom`. Directories for Mojo traits |
| 864 | should be named `mojom` (preferable) or `ipc`. Legacy names that are also |
| 865 | encountered are `public/interfaces`, `interfaces`, or just `mojo`. |
| 866 | |
| 867 | `mojom` is preferred for consistency between the directory name and the nested |
| 868 | namespace name. |
| 869 | |
| 870 | |
| 871 | ### Do not handle impossible situations |
| 872 | |
| 873 | Do not clutter the code by handling impossible situations. Omitting it makes |
| 874 | the invariants clear. This takes two different forms: |
| 875 | |
| 876 | * A less trustworthy process can be compromised by an adversary and send |
| 877 | arbitrary data. When processing data from a less trustworthy process, do |
| 878 | not attempt to handle this invalid data: just call |
| 879 | `mojo::ReportBadMessage()`. When invoked in the context of processing an |
| 880 | IPC from the renderer, this will kill the renderer process. |
| 881 | * A more trustworthy process must be trusted, by definition. Do not write |
| 882 | code to handle impossible situations "just in case" there's a bug. For |
| 883 | example, the renderer class `content::RenderFrameImpl` must always be |
| 884 | connected to certain control interfaces in the browser. It does not makes |
| 885 | sense to handle a Mojo connection error and try to reconnect: a connection |
| 886 | error signals that the browser process is in the process of deleting the |
| 887 | frame, and any attempt at reconnecting will never succeed. |
| 888 | |
| 889 | |
| 890 | ### Using mojo enums directly when possible |
| 891 | |
| 892 | `EnumTraits` generally do not add much value: incoming Mojo enum values are |
| 893 | already validated before typemapping, so it is guaranteed that the input value |
| 894 | to `EnumTraits<T>::FromMojom()` is already a valid enum value, so the method |
| 895 | itself is just a bunch of boilerplate to map between two very similarly named, |
| 896 | yet slightly different, enums. |
| 897 | |
| 898 | To avoid this, prefer to use the Mojo enum directly when possible. |
| 899 | |
| 900 | |
| 901 | ### Consider the cost of setting up message pipes |
| 902 | |
| 903 | Message pipes are fairly inexpensive, but they are not free either: it takes 6 |
| 904 | control messages to establish a message pipe. Keep this in mind: if the |
| 905 | interface is used relatively frequently, connecting once and reusing the |
| 906 | interface pointer is probably a good idea. |
| 907 | |
Alex Gough | 303f582 | 2022-01-27 05:35:16 | [diff] [blame] | 908 | ## Copy data out of BigBuffer before parsing |
| 909 | |
| 910 | [BigBuffer](mojo/public/mojom/base/big_buffer.mojom) uses shared memory to make |
| 911 | passing large messages fast. When shmem is backing the message, it may be |
| 912 | writable in the sending process while being read in the receiving process. If a |
| 913 | BigBuffer is received from an untrustworthy process, you should make a copy of |
| 914 | the data before processing it to avoid time-of-check time-of-use (TOCTOU) bugs. |
| 915 | The |size()| of the data cannot be manipulated. |
| 916 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 917 | |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 918 | ## Ensure An Explicit Grant For WebUI Bindings |
| 919 | |
| 920 | WebUI renderers sometimes need to call special, powerful IPC endpoints in a |
| 921 | privileged process. It is important to enforce the constraint that the |
| 922 | privileged callee previously created and blessed the calling process as a WebUI |
| 923 | process, and not as a (potentially compromised) web renderer or other |
| 924 | low-privilege process. |
| 925 | |
| 926 | * Use the standard pattern for instantiating `MojoWebUIController`. WebUI |
Chris Palmer | d40cda9 | 2017-12-04 21:12:46 | [diff] [blame] | 927 | Mojo interfaces must only be exposed through a `MojoWebUIController` subclass. |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 928 | * If there is external functionality that the WebUI needs, make sure to route |
| 929 | it through the Mojo interfaces implemented by the `MojoWebUIController`, to |
| 930 | avoid circumventing access checks. |
| 931 | |
| 932 | |
| 933 | ## Not-Yet-Shipped Features Should Be Feature-Checked On The Privileged Side |
| 934 | |
| 935 | Sometimes, there will be powerful new features that are not yet turned on by |
| 936 | default, such as behind a flag, Finch trial, or [origin |
| 937 | trial](https://www.chromium.org/blink/origin-trials). It is not safe to check |
| 938 | for the feature's availability on the renderer side (or in another low-privilege |
| 939 | process type). Instead, ensure that the check is done in the process that has |
| 940 | power to actually enact the feature. Otherwise, a compromised renderer could opt |
| 941 | itself in to the feature! If the feature might not yet be fully developed and |
| 942 | safe, vulnerabilities could arise. |
| 943 | |
| 944 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 945 | [security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc |
| 946 | [NfcTypeConverter.java]: https://chromium.googlesource.com/chromium/src/+/e97442ee6e8c4cf6bcf7f5623c6fb2cc8cce92ac/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java |
Oksana Zhuravlova | a77a9a1 | 2021-05-07 18:02:10 | [diff] [blame] | 947 | [mojo-doc-process-crashes]: https://chromium.googlesource.com/chromium/src/+/main/mojo/public/cpp/bindings#Best-practices-for-dealing-with-process-crashes-and-callbacks |
Lukasz Anforowicz | bb99a856 | 2019-01-25 19:19:38 | [diff] [blame] | 948 | [serialize-struct-tm-safely]: https://chromium-review.googlesource.com/c/chromium/src/+/679441 |