ipfs / ipfs/kubo

Audit Unsafe

Open
#4,485 8 comments 2 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
17.1k
Forks
3.2k
Avg merge
3d 18h
Merged PRs (30d)
11

Description

Given #4483, we need to audit all usages of unsafe. We generally avoid it but some of the libraries we use use it.

# Libraries

## go-os-rename

**UNSAFE**
https://github.com/ipfs/go-ipfs/issues/4808#issuecomment-375813269
~~However, it appears to not handle long paths, unlike the built-in one. Furthermore, it looks like go now replaces existing files on windows (i.e., the reason we use this library no longer applies). We should stop using it.~~

## go-sockaddr

**SAFE?**

Mostly exported from go-ipfs. However, I've asked the reporter in #4483 to try disabling it to see if that fixes it.

## go-peerstream

**SAFE**

Used to compare pointers to avoid a deadlock.

## websocket

**SAFE**

Used to do a fast XOR. Looks safe.

## bbloom

**UNSAFE?**

Used all over the place and go vet complains that it's used incorrectly. I don't believe we use bbloom in any critical places so we can probably switch to bloom (made by the same author without unsafe).

## gogo-protobuf

**UNKNOWN**

Unclear but probably well vetted. I'm in the process of upgrading this library anyways (so if there are any missing bug fixes, that should pull them in).

## fuse

**LOL**

## go-net

**SAFE?**

Made by the go authors. Regardless we should update this.

## fsnotify.v1

**SAFE**

Looks fine.

## sys

**SAFE?**

We should try to upgrade to the latest version everywhere anyways.

## Badger

**UNKNOWN**

Their mmap code looks fine. I'm mostly worried about the skip-list implementation. However, I doubt the user in #4483 has badger enabled.

## go-codec

**UNKNOWN**

## murmur3

**UNKNOWN**

However, we don't use this so it's definitely not causing any issues.

On the other hand, it looks like it may be doing unaligned loads (problem on arm?).

## go-colorable

**SAFE**

Used on windows for syscalls. Definitely safe.

## go-logging

**SAFE?**

The memory backend uses unsafe. However, it looks like it's just using it to atomically swap pointers. From what I can tell, this all looks safe.

## go-crypto

**SAFE?**

Probably safe. Might as well update.

## leveldb

**UNKNOWN**

Uh.... Yeah. This one's going to be hard to audit. We should just update it and hope.

## go-text

**UNLIKELY**

Written by the go authors. However, we should update it.

## pb (progress bar)

**SAFE**

Only uses unsafe for a few simple ioctls. Unlikely to be an issue.

## GoEndian

**SAFE**

Checked.

## hang-fds

**WHO CARES**

For testing, looks fine bug I don't particularly care.

# TODO

* ~[ ] Check if disabling fuse fixes #4483~
* [x] Check if disabling reuseport fixes #4483
* [ ] Switch to a better fuse library.
* [x] Upgrade gogo-protobuf.
* [x] Upgrade go-net.
* [x] Switch to safe bbloom (if fast enough). Otherwise, check bbloom.
* Fixed bbloom
* [ ] Get badger to use a safe bloom filter implementation (https://github.com/dgraph-io/badger/issues/1005)
* [x] Make sure to upgrade everything to the latest sys.
* [x] Check badger's skip-list (and probably give up...).
* This has been audited.
* [x] Update go-codec.
* [x] Update go-crypto.
* [x] Stop using go-os-rename
* [x] Update goleveldb
* [x] Update go-text

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.