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