AddBookmarks panics given bookmark with empty but non-nil children slice

[jo3-l]

Hi, thanks for your work on this great tool. I'm using the pdfcpu API (latest release, 0.4.2) and hit a minor snag with the AddBookmarks/AddBookmarksFile routines. The issue appears to be independent of the input PDF — it occurred for all the files I tried it with.

Summary

The following call to api.AddBookmarks works as expected:

bm := pdfcpu.Bookmark{
	PageFrom: 1,
	Title: "foobar",
}
err := api.AddBookmarksFile("in.pdf", "out.pdf", []pdfcpu.Bookmark{bm}, true, nil)
if err != nil {
	panic(err)
}

but the following addition (setting Children to an empty but non-nil slice) results in a runtime error, panic: runtime error: invalid memory address or nil pointer dereference:

bm := pdfcpu.Bookmark{
	PageFrom: 1,
	Title: "foobar",
+	Children: []pdfcpu.Bookmark{},
}
err := api.AddBookmarksFile("in.pdf", "out.pdf", []pdfcpu.Bookmark{bm}, true, nil)
if err != nil {
	panic(err)
}

Show full stack trace

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x70272a]

goroutine 1 [running]:
github.com/pdfcpu/pdfcpu/pkg/pdfcpu.createOutlineItemDict(0xc0001ea4e0, {0xc0000adf20, 0x1, 0x4?}, 0xc000fecf20, 0x0)
        [...]/github.com/pdfcpu/pdfcpu@v0.4.2/pkg/pdfcpu/bookmark.go:301 +0x2aa
github.com/pdfcpu/pdfcpu/pkg/pdfcpu.AddBookmarks(0xc0001ea4e0, {0xc0000adf20, 0x1, 0x1}, 0x1)
        [...]/github.com/pdfcpu/pdfcpu@v0.4.2/pkg/pdfcpu/bookmark.go:355 +0x15e
github.com/pdfcpu/pdfcpu/pkg/api.AddBookmarks({0x8c0180, 0xc000014518}, {0x8bea00, 0xc000014520}, {0xc0000adf20, 0x1, 0x1}, 0xc8?, 0x7fe009fe2108?)
        [...]/github.com/pdfcpu/pdfcpu@v0.4.2/pkg/api/bookmark.go:51 +0x1f1
github.com/pdfcpu/pdfcpu/pkg/api.AddBookmarksFile({0x81d153, 0x6}, {0x81d962, 0x7}, {0xc0001a5f20, 0x1, 0x1}, 0xbb?, 0xc0000061a0?)
        [...]/github.com/pdfcpu/pdfcpu@v0.4.2/pkg/api/bookmark.go:107 +0x291
main.main()
        [...]/main.go:18 +0xb4
exit status 2

Reproduction Steps

1. Create an empty Go module in a new directory.
2. Install the latest version of pdfcpu:

go get github.com/pdfcpu/pdfcpu
go mod tidy

3. Add the following code to main.go; this is the same as the code provided above with the relevant imports added so it can be run directly.

Show code

package main

import (
	"fmt"

	"github.com/pdfcpu/pdfcpu/pkg/api"
	"github.com/pdfcpu/pdfcpu/pkg/pdfcpu"
)

func main() {
	bms := []pdfcpu.Bookmark{
		{
			PageFrom: 1,
			Title:    "foobar",
			// Children: []pdfcpu.Bookmark{},
		},
	}
	err := api.AddBookmarksFile("in.pdf", "out.pdf", bms, true, nil)
	if err != nil {
		panic(err)
	}
	fmt.Println("ok")
}

4. Download any PDF and save it to in.pdf; for this issue report I used CenterOfWhy.pdf from your test suite: https://github.com/pdfcpu/pdfcpu/blob/master/pkg/testdata/CenterOfWhy.pdf.
5. Execute go run .; observe that no runtime error occurs.
6. Uncomment Children: []pdfcpu.Bookmark{} and execute go run . again; observe the runtime error.

Analysis

The source of the issue seems to be the createOutlineItemDict function:

pdfcpu/pkg/pdfcpu/bookmark.go

Line 259 in a9afcfe

func createOutlineItemDict(ctx *model.Context, bms []Bookmark, parent *types.IndirectRef, parentPageNr *int) (*types.IndirectRef, *types.IndirectRef, int, int, error) {

Specifically, for each bookmark bm given, createOutlineItemDict checks if bm.Children is non-nil and if so recurses on bm.Children:

pdfcpu/pkg/pdfcpu/bookmark.go

Lines 294 to 296 in a9afcfe

	if bm.Children != nil {
	first, last, c, visc, err := createOutlineItemDict(ctx, bm.Children, ir, &bm.PageFrom)

However, this seems problematic if bm.Children is a non-nil empty slice: createOutlineItemDict(..., bm.Children, ...) will return two nil IndirectRefs, so first and last will be nil. This then causes the observed panic when first and last are later dereferenced:

pdfcpu/pkg/pdfcpu/bookmark.go

Lines 301 to 302 in a9afcfe

	d["First"] = *first
	d["Last"] = *last

and indeed the stack trace points to line 301 as well. The likely fix (untested) is to change bm.Children != nil to len(bm.Children) == 0.

Context

This issue has an easy workaround; just always pass nil slices instead of empty but non-nil ones. However, in my use-case, this is minorly inconvenient: the bookmarks are generated dynamically, and so I need a bit of extra code to handle this scenario:

if len(out.Children) == 0 {
	out.Children = nil
}

Since the fix appears reasonably simple, I think it's worth fixing this issue — unless, of course, there's a reason for not allowing empty slices that I'm missing.

Thanks!

[hhrutter]

Fixed with latest commit.