Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent segfault after mmap failure #58

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion db.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,12 @@ func (db *DB) mmap(minsz int) error {

// Memory-map the data file as a byte slice.
if err := mmap(db, size); err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous returned error will cause the db library to panic? if that is the case, shouldnt we fix that behavior instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like if db fails on mmap, then all following calls into it should return mmap failure error instead of causing pancis?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the reason is panics after returning the error is that calling unmap invalidates db.meta0 and/or db.meta1. So we could also fix this by modifying the logic to never access those variables if mmap fails, and put the db in an "error" state, as you suggested.

That's probably a better solution than attempting to remap the old db. Just recently we received a bug report where it seems the remap failed as well, causing a panic. The one advantage of remapping is that you can still read from the db, you just can't write any more. But seeing as it's a failure state anyway, I don't think being able to read the db adds a ton of value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukechampine

You are right. I am fine with your current solution as I mentioned. But, I want to make sure this does fix the original issue. Also we need to have sort of a test for this ideally.

thank you!

// mmap failed; the system may have run out of space. Fallback to
// mapping the bare minimum needed for the current db size.
if err2 := mmap(db, db.datasz); err2 != nil {
panic(fmt.Sprintf("failed to revert db size after failed mmap: %v", err2))
}
return MmapError(err.Error())
}

// Save references to the meta pages.
Expand Down
7 changes: 7 additions & 0 deletions errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,10 @@ var (
// non-bucket key on an existing bucket key.
ErrIncompatibleValue = errors.New("incompatible value")
)

// MmapError represents an error resulting from a failed mmap call. Typically,
// this error means that no further database writes will be possible. The most
// common cause is insufficient disk space.
type MmapError string

func (e MmapError) Error() string { return string(e) }