PDFium Security

Basic Info

Fixing bugs? Please get your CLs reviewed by 1 of {jorgelo,leecam,palmer,rsesek,tsepez}@chromium.org and 1 of {bo_xu,jun_fang}@foxitsoftware.com.

Integer Overflow

We want to standardize on handling integer overflows by:

Preferring new[] and new instead of calloc, wherever possible.

In places where the code is not ready to be turned into idiomatic C++, preferring calloc to malloc; definitely prefer calloc to malloc + memset.

Preferring CheckedNumeric<T> to ad hoc checks.

For convenience, use the various typedefs for clarity, e.g. typedef base::CheckedNumeric<FX_DWORD> FX_SAFE_DWORD;. If you need more typedefs like this, or if you need them more widely visible, don't hesitate to make the change.

Yes, that might look odd. Currently, the codebase mixes C++ and C memory allocation, and for now it is more important to fix bugs than to change the coding style of a given area of the code. Ultimately, we'd like to get the code to idiomatic C++11, but we're going to get there incrementally.

Uninitialized Memory References

We want to standardize on handling uninitialized memory references with:

Default constructors that do the right thing

Explicit initial values

Git Workflow

Let's use "git cl upload --private" since the bugs are private. The fixes we've done so far all point directly to exploitable vulnerabilities, so there isn't much point in keeping the bugs private if we make the CLs public. In Chromium, we live with public CLs even for private bugs because we have to for the commit queue and the trybots to work. In PDFium, we have no trybots (yet) and no commit queue (yet?), so we might as well take advantage of --private while we can.

But, hopefully soon, we'll have bots and --private won't be feasible. That is OK. We'll stop using --private at that time.

The top line/subject line of the commit message should always be as explicit as possible. Not just "fix bug", but "Fix UAF in ModulateFooContainer" or "Fix UMR in thing::DoStuff".

Future Desiderata

We should develop some rough equivalent of Blink's LayoutTests, to avoid layout regressions while fixing bugs. In Blink, we can have some confidence that fixes don't break layout because we can run all the LayoutTests before landing the fix. At a minimum, it'd be nice to have at least bitmap-matching tests and a corpus of PDFs to test each CL against. Ideally, there could be some test automation for the layout engine like Blink has. (And we could even commit new tests along with our bug fixes.)

We should also develop some kind of performance test suite: rendering time over the corpus of test PDFs, memory used when rendering the corpus, and binary size.

Default constructors for all structs and classes, and use new/new[] everywhere instead of malloc and calloc.

No more non-const references (especially when used as out-parameters).