blob: 5c51e8b9b28c6a59707a83719852ba1ed2438da8 [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
Darwin Huangb4bd2452019-10-08 22:56:04167> Each `InterfaceProvider` for frames and workers is strongly associated with an
168> origin. Where possible, prefer to use this associated origin rather than
169> sending it over IPC. (See <https://crbug.com/734210> and
170> <https://crbug.com/775792/>).
Daniel Chengd21b551f2017-10-30 08:43:23171
172For example, the browser process must not (fully) trust the renderer's claims
173about origins. The browser process should already know what origin the renderer
174is evaluating, and thus should already have this data (for example, see
175`RenderFrameHost::GetLastCommittedOrigin()`). Thus, a method that requires
176passing an origin from the renderer to the browser process has a conceptual
177error, and quite possibly, a vulnerability.
178
179> Note: there are currently subtle races when using `GetLastCommittedOrigin()`
180> that will be resolved by fixing <https://crbug.com/729021>.
181
182Similarly, the browser process must not trust the renderer's claims about file
183pathnames. It would be unsafe for the browser process to save a downloaded file
184to `~/.bashrc` just because the renderer asked. Instead, it would be better for
185the browser process to:
186
1871. Kill the renderer if `basename(pathname) != pathname`, since the renderer is
188 obviously compromised if it makes this mistake.
1891. Defang the basename, by removing leading dots, et cetera. Note that the
190 definition of proper defanging varies per platform.
1911. Prepend its own parent directory to the basename, e.g. ~/Downloads.
192
193> TODO(https://crbug.com/779196): Even better would be to implement a C++ type
194> performs the appropriate sanitizations and recommend its usage directly here.
195
196
Lukasz Anforowiczbb99a8562019-01-25 19:19:38197### Validate privilege-presuming data received over IPC
198
199If it is not possible to avoid sending privilege-presuming data over IPC (see
200the previous section), then such data should be verified before being used.
201
202* Browser process:
203 - Use `ChildProcessSecurityPolicy`'s methods like
204 `CanAccessDataForOrigin` or `CanReadFile` to verify IPC messages
205 received from less privileged processes.
206 - When verification fails, ignore the IPC and terminate the renderer process
207 using `mojo::ReportBadMessage` (or using `mojo::GetBadMessageCallback` for
208 messages handled asynchronously). For legacy IPC, the renderer process
209 may be terminated by calling the `ReceivedBadMessage` function (separate
210 implementations exist for `//content`, `//chrome` and other layers).
211
212* NetworkService process:
213 - Do not trust `network::ResourceRequest::request_initiator` - verify it
214 using `VerifyRequestInitiatorLock` and fall back to a fail-safe origin
215 (e.g. an opaque origin) when verification fails.
216
217
Daniel Cheng0a7a4ec2018-02-13 23:30:27218### Do not define unused or unimplemented things
219
220Mojo interfaces often cross privilege boundaries. Having well-defined interfaces
221that don't contain stubbed out methods or unused parameters makes it easier to
Daniel Chengcda1df5b2018-03-30 21:30:16222understand and evaluate the implications of crossing these boundaries. Several
223common areas to watch out for:
Daniel Cheng0a7a4ec2018-02-13 23:30:27224
225
226#### Do use EnableIf to guard platform-specific constructs
Daniel Chengd21b551f2017-10-30 08:43:23227
228Platform-specific functionality should only be defined on the platforms where
Daniel Cheng0a7a4ec2018-02-13 23:30:27229it is implemented. Use the Mojo `EnableIf` annotation to guard definitions that
230should only be visible in certain build configurations.
Daniel Chengd21b551f2017-10-30 08:43:23231
Daniel Cheng0a7a4ec2018-02-13 23:30:27232**_Good_**
233```c++
234// GN file:
235mojom("view_bindings") {
236 // ...
Daniel Chengd21b551f2017-10-30 08:43:23237
Daniel Cheng0a7a4ec2018-02-13 23:30:27238 enabled_features = []
239 if (is_android) {
Bill Orrdc01ca32019-01-30 00:22:11240 enabled_features += [ "is_android" ]
Daniel Cheng0a7a4ec2018-02-13 23:30:27241 }
242}
243
244// mojom definition:
245interface View {
246 // ...
247
248 [EnableIf=is_android]
249 UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
250 bool animate);
251};
252
253// C++ implementation:
254class View : public mojom::View {
255 public:
256 // ...
257
258#if defined(OS_ANDROID)
259 void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
260 bool animate);
261#endif
262};
263```
264
265**_Bad_**
266```c++
267// mojom definition:
268interface View {
269 // ...
270
271 UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
272 bool animate);
273};
274
275// C++ implementation:
276class View : public mojom::View {
277 public:
278 // ...
279
280#if defined(OS_ANDROID)
281 void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
282 bool animate) override;
283#else
284 void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
285 bool animate) override {
286 NOTREACHED();
287 }
288#endif
289};
290```
291
Xiaohan Wang688f49d2018-02-26 19:39:53292The `EnableIf` annotation can be applied to almost anything: imports,
293interfaces, methods, arguments, constants, structs, struct members, enums,
294enumerator values, et cetera.
Daniel Cheng0a7a4ec2018-02-13 23:30:27295
296
297#### Do not define unimplemented methods
298
299Reviewing IPC requires reviewing a concrete implementation of the Mojo
300interface, to evaluate how the (possibly untrustworthy) inputs are used, what
301outputs are produced, et cetera. If a method is not yet implemented, do not
302define it in the interface.
303
304**_Bad_**
305```c++
306// mojom definition:
307interface Spaceship {
308 EnterHyperspace();
309 ExitHyperspace();
310};
311
312// C++ implementation:
313class SpaceshipPrototype : public mojom::Spaceship {
314 void EnterHyperspace() { /* TODO(dcheng): Implement. */ }
315 void ExitHyperspace() { /* TODO(dcheng): Implement. */ }
316};
317```
318
319
Daniel Chengcda1df5b2018-03-30 21:30:16320#### Do not define placeholder enumerator values
Daniel Cheng0a7a4ec2018-02-13 23:30:27321
Daniel Chengcda1df5b2018-03-30 21:30:16322Do not define placeholder enumerator values like `kLast`, `kMax`, `kCount`, et
323cetera. Instead, rely on the autogenerated `kMaxValue` enumerator emitted for
324Mojo C++ bindings.
Daniel Chengd21b551f2017-10-30 08:43:23325
Daniel Chengcda1df5b2018-03-30 21:30:16326For UMA histograms, logging a Mojo enum is simple: simply use the two argument
327version of `UMA_HISTOGRAM_ENUMERATION`:
Daniel Chengd21b551f2017-10-30 08:43:23328
Daniel Chengcda1df5b2018-03-30 21:30:16329**_Good_**
330```c++
331// mojom definition:
332enum GoatStatus {
333 kHappy,
334 kSad,
335 kHungry,
336 kGoaty,
337};
338
339// C++:
340UMA_HISTOGRAM_ENUMERATION("Goat.Status", status);
341```
342
343Using a `kCount` sentinel complicates `switch` statements and makes it harder to
344enforce invariants: code needs to actively enforce that the otherwise invalid
345`kCount` sentinel value is not incorrectly passed around.
346
347**_Bad_**
348```c++
349// mojom definition:
350enum CatStatus {
351 kAloof,
352 kCount,
353};
354
355// C++
356switch (cat_status) {
357 case CatStatus::kAloof:
358 IgnoreHuman();
359 break;
360 case CatStatus::kCount:
361 // this should never happen
362}
363```
364
365Defining `kLast` manually results in ugly casts to perform arithmetic:
366
367**_Bad_**
368```c++
369// mojom definition:
370enum WhaleStatus {
371 kFail,
372 kNotFail,
373 kLast = kNotFail,
374};
375
376// C++:
377UMA_HISTOGRAM_ENUMERATION("Whale.Status", status,
378 static_cast<int>(WhaleStatus::kLast) + 1);
379```
380
381For interoperation with legacy IPC, also use `kMaxValue` rather than defining a
382custom `kLast`:
383
384**_Good_**
385```c++
386IPC_ENUM_TRAITS_MAX_VALUE(GoatStatus, GoatStatus::kMaxValue);
387```
Daniel Chengd21b551f2017-10-30 08:43:23388
389
390### Use structured types
391
392Where possible, use structured types: this allows the type system to help
393enforce that the input data is valid. Common ones to watch out for:
394
Oksana Zhuravlovac70ec7ef2018-03-13 21:08:42395* Files: use `mojo_base.mojom.File`, not raw descriptor types like `HANDLE`
Daniel Chengd21b551f2017-10-30 08:43:23396 and `int`.
Oksana Zhuravlovac70ec7ef2018-03-13 21:08:42397* File paths: use `mojo_base.mojom.FilePath`, not `string`.
Oksana Zhuravlova38321d082018-04-27 23:59:01398* JSON: use `mojo_base.mojom.Value`, not `string`.
Daniel Chengd21b551f2017-10-30 08:43:23399* Mojo interfaces: use `Interface` or `Interface&`, not `handle` or
400 `handle<message_pipe>`.
Oksana Zhuravlova34579e912018-03-23 00:18:49401* Nonces: use `mojo_base.mojom.UnguessableToken`, not `string`.
Daniel Chengd21b551f2017-10-30 08:43:23402* Origins: use `url.mojom.Origin`, not `url.mojom.Url` and certainly not
403 `string`.
Oksana Zhuravlova57e8dca2018-03-20 03:21:34404* Time types: use `mojo_base.mojom.TimeDelta` /
405 `mojo_base.mojom.TimeTicks` / `mojo_base.mojom.Time`, not `int64` /
Daniel Chengd21b551f2017-10-30 08:43:23406 `uint64` / `double` / et cetera.
407* URLs: use `url.mojom.Url`, not `string`.
Daniel Cheng876c66322018-08-29 06:42:31408* `array<uint8>` or `string` and `memcpy()`: use a Mojo struct and statically
409 define the serialized fields. While `memcpy()` may be tempting for its
410 simplicity, it can leak info in padding. Even worse, `memcpy()` can easily
411 copy [undocumented fields][serialize-struct-tm-safely] or newly introduced
412 fields that were never evaluated for safety by the developer or reviewer.
Daniel Chengd21b551f2017-10-30 08:43:23413
414**_Good_**
415
416```c++
417interface ReportingService {
Oksana Zhuravlova57e8dca2018-03-20 03:21:34418 ReportDeprecation(mojo_base.mojom.TimeTicks time,
Daniel Chengd21b551f2017-10-30 08:43:23419 url.mojom.Url resource,
420 uint32 line_number);
421};
422```
423
424**_Bad_**
425
426```c++
427interface ReportingService {
428 // Bad: unclear what units |time| is or what |data| contains.
Oksana Zhuravlova38321d082018-04-27 23:59:01429 ReportDeprecation(double time, mojo_base.mojom.Value data);
Daniel Chengd21b551f2017-10-30 08:43:23430};
431```
432
Daniel Cheng876c66322018-08-29 06:42:31433Avoid parallel arrays of data that require the receiver to validate that the
434arrays have matching lengths. Instead, bundle the data together in a struct so
435it is impossible to have a mismatch:
Daniel Chengd21b551f2017-10-30 08:43:23436
437**_Good_**
438
439```c++
440struct Pixel {
441 int8 reds;
442 int8 greens;
443 int8 blues;
444 int8 alphas;
445};
446
447struct Bitmap {
448 // Good: it is impossible for there to be mismatched data.
449 array<Pixel> pixels;
450};
451```
452
453**_Bad_**
454
455```c++
456// Bad: code using this struct will need to validate that all the arrays have
457// matching sizes.
458struct Bitmap {
459 array<int8> reds;
460 array<int8> greens;
461 array<int8> blues;
462 array<int8> alphas;
463};
464```
465
466
467### Beware of arithmetic overflow
468
469> TODO(dcheng): Import the guidance from the legacy IPC doc.
470
471Signed overflow is undefined in C++. If unsure about whether or not something
472will overflow, use the safe numeric helpers from `//base/numerics`!
473
474**_Good_**
475
476```c++
477base::CheckedNumeric<int32_t> size = mojo_rect->width();
478size *= mojo_rect.height();
479if (!size.IsValid()) {
480 mojo::ReportBadMessage("Bad size from renderer!");
481}
482```
483
484**_Bad_**
485
486```c++
487// Bad: Signed overflow is undefined in C++!
488int32_t size = mojo_rect->width() * mojo_rect.height();
489```
490
491Note that even if the types have defined overflow semantics, it is almost always
492a good idea to check for overflow.
493
494**_Good_**
495
496```c++
497uint32_t alloc_size;
498if (!CheckMul(request->elements(), request->element_size())
499 .AssignIfValid(&alloc_size)) {
500 // Safe: avoids allocating with a bogus size that overflowed to a smaller than
501 // expected value.
Rayan Kanso81aeee32019-07-11 18:50:44502 mojo::ReportBadMessage("Invalid allocation size");
Daniel Chengd21b551f2017-10-30 08:43:23503}
504
505Element* array = CreateArray(alloc_size);
506for (size_t i = 0; i < request->element_size(); ++i) {
507 array[i] = PopulateArray(i);
508}
509```
510
511**_Bad_**
512
513```c++
514uint32_t alloc_size = request->elements() * request->element_size();
515
516// Dangerous: alloc_size can overflow so that CreateArray allocates too little
517// memory. Subsequent assignments will turn into an out-of-bound write!
518Element* array = CreateArray(alloc_size);
519for (size_t i = 0; i < request->element_size(); ++i) {
520 array[i] = PopulateArray(i);
521}
522```
523
524
525## C++ Best Practices
526
527
Daniel Chengd8df82002018-02-09 20:52:15528### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler sparingly
Daniel Cheng9ae28e02018-01-17 18:44:10529
530Mojo provides several convenience helpers to automatically invoke a callback if
531the callback has not already been invoked in some other way when the callback is
532destroyed, e.g.:
533
534```c++
535 {
536 base::Callback<int> cb = mojo::WrapCallbackWithDefaultInvokeIfNotRun(
537 base::Bind([](int) { ... }), -1);
538 } // |cb| is automatically invoked with an argument of -1.
539```
540
Daniel Chengd8df82002018-02-09 20:52:15541This can be useful for detecting interface errors:
Daniel Cheng9ae28e02018-01-17 18:44:10542
Daniel Chengd8df82002018-02-09 20:52:15543```c++
544 process->GetMemoryStatistics(
545 mojo::WrapCallbackWithDefaultInvokeIfNotRun(
546 base::Bind(&MemoryProfiler::OnReplyFromRenderer), <failure args>));
547 // If the remote process dies, &MemoryProfiler::OnReplyFromRenderer will be
548 // invoked with <failure args> when Mojo drops outstanding callbacks due to
549 // a connection error on |process|.
550```
551
552However, due to limitations of the current implementation, it's difficult to
553tell if a callback object has invoke-on-destroy behavior. In general:
554
5551. Prefer error connection handlers where possible.
5561. Only use the callback helpers for detecting interface errors. These
557 callbacks may be invoked during destruction and must carefully consider
558 receiver object lifetime. For more information, please see the
559 [Mojo documentation][mojo-doc-process-crashes].
560
561> Note that using the callback wrappers in the renderer is often unnecessary.
562> Message pipes are typically closed as part of a Document shutting down; since
563> many Blink objects already inherit `blink::ContextLifecycleObserver`, it is
564> usually more idiomatic to use this signal to perform any needed cleanup work.
Daniel Cheng9ae28e02018-01-17 18:44:10565
Daniel Chengd21b551f2017-10-30 08:43:23566### Use StructTraits
567
568Creating a typemap and defining a `StructTraits` specialization moves the
569complexity of serialization, deserialization, and validation into a central
570location. We universally recommend this over defining `TypeConverter`
571specializations: when a value fails deserialization, the receiver method will
572never even be invoked. As a bonus, it also reduces the number of copies during
573serialization and deserialization. 😄
574
575**_Good_**
576
577```c++
Daniel Cheng1f386932018-01-29 19:56:47578// In url_gurl_mojom_traits.h:
Daniel Chengd21b551f2017-10-30 08:43:23579template <>
580struct StructTraits<url::mojom::UrlDataView, GURL> {
581 static base::StringPiece url(const GURL& r);
582
583 // If Read() returns false, Mojo will discard the message.
584 static bool Read(url::mojom::UrlDataView data, GURL* out);
585};
586
Daniel Cheng1f386932018-01-29 19:56:47587// In url_gurl_mojom_traits.cc:
Daniel Chengd21b551f2017-10-30 08:43:23588// Note that methods that aren't simple getters should be defined
589// out-of-line to avoid code bloat.
590base::StringPiece StructTraits<url::mojom::UrlDataView, GURL>::url(
591 const GURL& r) {
592 if (r.possibly_invalid_spec().length() > url::kMaxURLChars ||
593 !r.is_valid()) {
594 return base::StringPiece();
595 }
596 return base::StringPiece(r.possibly_invalid_spec().c_str(),
597 r.possibly_invalid_spec().length());
598}
599
600bool StructTraits<url::mojom::UrlDataView, GURL>::Read(
601 url::mojom::UrlDataView data, GURL* out) {
602 base::StringPiece url_string;
603 if (!data.ReadUrl(&url_string))
604 return false;
605 if (url_string.length() > url::kMaxURLChars)
606 return false;
607 *out = GURL(url_string);
608 return !url_string.empty() && out->is_valid();
609}
610```
611
612**_Bad_**
613
614```c++
615template <>
616struct TypeConverter<url::mojom::UrlPtr, GURL> {
617 // Inefficient: this copies data once off the wire to create a
618 // url.mojom.Url object, then copies it again to create a GURL.
619 static GURL Convert(const url::mojom::UrlPtr url) {
620 if (url.url.is_empty()) return GURL();
621 // Not good: no way to signal errors, so any code that converts the
622 // Mojo struct to a GURL will somehow need to check for errors…
623 // but it can't even be distinguished from the empty URL case!
624 if (url.url.size() > url::kMaxURLChars) return GURL();
625 return GURL(url.url);
626 }
627};
628```
629
630There are also corresponding `EnumTraits` and `UnionTraits` specializations for
631mojo enums and unions respectively.
632
633
634### StructTraits getters should be simple
635
636Where possible, `StructTraits` should be returning const references or simple
637read-only views of the data. Having to create temporary data structures during
638serialization should be rare, and it should be even rarer to mutate the input
639argument.
640
641
642### Out-of-line complex serialization/deserialization logic
643
644A `StructTraits` specialization is almost always fully specialized. Only define
645`StructTraits` methods inline in the header if the method is a simple getter
646that returns a reference, pointer, or other simple POD. Define all other
647methods out-of-line to avoid code bloat.
648
649
650### Do not write one-off functions to convert to/from Mojo types
651
652There are some instances where it is simply not possible to define a
653`StructTraits` for type mapping: this commonly occurs with Blink IDL and Oilpan
654types. In these instances, add a `TypeConverter` specialization rather than
655defining a one-off conversion function. This makes it easier to search for and
656audit code that does potentially risky type conversions.
657
658> The use of `TypeConverter` should be limited as much as possible: ideally,
659> only use it in renderers.
660
661**_Good_**
662
663```c++
664template <>
665struct TypeConverter<IDLDictionary, mojom::blink::DictionaryPtr> {
666 static IDLDictionary* Convert(const mojom::blink::DictionaryPtr& in) {
667 // Note that unlike StructTraits, there is no out-of-band way to signal
668 // failure.
669 IDLDictionary* out = new IDLDictionary;
670 out->int_value = in->int_value;
671 out->str_value = in->str_value;
672 return out;
673 }
674};
675```
676
677**_Bad_**
678
679```c++
680// Using a custom one-off function makes this hard to discover in security
681// audits.
682IDLDictionary* FromMojo(const mojom::blink::DictionaryPtr& in) {
683 IDLDictionary* out = new IDLDictionary;
684 out->int_value = in->int_value;
685 out->str_value = in->str_value;
686 return out;
687}
688```
689
690
691### Use the proper abstractions
692
Oksana Zhuravlova9f3b8ef2019-08-26 20:27:40693`mojo::ReceiverSet` implies multiple clients may connect. If this actually isn't
Daniel Chengd21b551f2017-10-30 08:43:23694the case, please do not use it. For example, if an interface can be rebound,
Oksana Zhuravlova9f3b8ef2019-08-26 20:27:40695then use the singular `mojo::Receiver` and simply `reset()` the existing receiver
Daniel Chengd21b551f2017-10-30 08:43:23696before reusing it.
697
698
Daniel Chengf48fd972017-11-21 19:15:53699### Explicitly reject bad input
700
701While validation should be done inside `StructTraits` specializations when
702possible, there are situations where additional checks, e.g. overflow checks,
703are needed outside of `StructTraits` specializations. Use
704`mojo::ReportBadMessage()` or `mojo::GetBadMessageCallback()` to reject bad
705input in these situations. Under the hood, this may record UMAs, kill the
706process sending bad input, et cetera.
707
708* `mojo::ReportBadMessage()`: use to report bad IPC input while a message is
709 being dispatched on the stack.
710* `mojo::GetBadMessageCallback()`: use to generate a callback to report bad
711 IPC input. The callback must be generated while a message is being
712 dispatched on the stack; however, the returned callback may be invoked be
713 freely invoked in asynchronously posted callbacks.
714
715
Daniel Chengd21b551f2017-10-30 08:43:23716## Java Best Practices
717
718Unfortunately, there are no strongly established conventions here. Most code
719tends to write manual conversion helpers and throw an exception on conversion
720failure. See [NfcTypeConverter.java] as one example of how to write conversion
721code.
722
723
724## General Code Health
725
726
727### Naming Conventions
728
729Place mojo types in `<top-level namespace>.mojom`. Directories for Mojo traits
730should be named `mojom` (preferable) or `ipc`. Legacy names that are also
731encountered are `public/interfaces`, `interfaces`, or just `mojo`.
732
733`mojom` is preferred for consistency between the directory name and the nested
734namespace name.
735
736
737### Do not handle impossible situations
738
739Do not clutter the code by handling impossible situations. Omitting it makes
740the invariants clear. This takes two different forms:
741
742* A less trustworthy process can be compromised by an adversary and send
743 arbitrary data. When processing data from a less trustworthy process, do
744 not attempt to handle this invalid data: just call
745 `mojo::ReportBadMessage()`. When invoked in the context of processing an
746 IPC from the renderer, this will kill the renderer process.
747* A more trustworthy process must be trusted, by definition. Do not write
748 code to handle impossible situations "just in case" there's a bug. For
749 example, the renderer class `content::RenderFrameImpl` must always be
750 connected to certain control interfaces in the browser. It does not makes
751 sense to handle a Mojo connection error and try to reconnect: a connection
752 error signals that the browser process is in the process of deleting the
753 frame, and any attempt at reconnecting will never succeed.
754
755
756### Using mojo enums directly when possible
757
758`EnumTraits` generally do not add much value: incoming Mojo enum values are
759already validated before typemapping, so it is guaranteed that the input value
760to `EnumTraits<T>::FromMojom()` is already a valid enum value, so the method
761itself is just a bunch of boilerplate to map between two very similarly named,
762yet slightly different, enums.
763
764To avoid this, prefer to use the Mojo enum directly when possible.
765
766
767### Consider the cost of setting up message pipes
768
769Message pipes are fairly inexpensive, but they are not free either: it takes 6
770control messages to establish a message pipe. Keep this in mind: if the
771interface is used relatively frequently, connecting once and reusing the
772interface pointer is probably a good idea.
773
774
Chris Palmer76482e62017-12-04 21:05:06775## Ensure An Explicit Grant For WebUI Bindings
776
777WebUI renderers sometimes need to call special, powerful IPC endpoints in a
778privileged process. It is important to enforce the constraint that the
779privileged callee previously created and blessed the calling process as a WebUI
780process, and not as a (potentially compromised) web renderer or other
781low-privilege process.
782
783* Use the standard pattern for instantiating `MojoWebUIController`. WebUI
Chris Palmerd40cda92017-12-04 21:12:46784Mojo interfaces must only be exposed through a `MojoWebUIController` subclass.
Chris Palmer76482e62017-12-04 21:05:06785* If there is external functionality that the WebUI needs, make sure to route
786it through the Mojo interfaces implemented by the `MojoWebUIController`, to
787avoid circumventing access checks.
788
789
790## Not-Yet-Shipped Features Should Be Feature-Checked On The Privileged Side
791
792Sometimes, there will be powerful new features that are not yet turned on by
793default, such as behind a flag, Finch trial, or [origin
794trial](https://www.chromium.org/blink/origin-trials). It is not safe to check
795for the feature's availability on the renderer side (or in another low-privilege
796process type). Instead, ensure that the check is done in the process that has
797power to actually enact the feature. Otherwise, a compromised renderer could opt
798itself in to the feature! If the feature might not yet be fully developed and
799safe, vulnerabilities could arise.
800
801
Daniel Chengd21b551f2017-10-30 08:43:23802[security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
803[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:15804[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:38805[serialize-struct-tm-safely]: https://chromium-review.googlesource.com/c/chromium/src/+/679441