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

Audit Unsafe #4485

Open
Stebalien opened this Issue Dec 13, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@Stebalien
Contributor

Stebalien commented Dec 13, 2017

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
#4808 (comment)
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
  • Check if disabling reuseport fixes #4483
  • Switch to a better fuse library.
  • Upgrade gogo-protobuf.
  • Upgrade go-net.
  • Switch to safe bbloom (if fast enough). Otherwise, check bbloom.
  • Make sure to upgrade everything to the latest sys.
  • Check badger's skip-list (and probably give up...).
  • Update go-codec.
  • Update go-crypto.
  • Stop using go-os-rename
  • Update goleveldb
  • Update go-text
@Kubuxu

This comment has been minimized.

Member

Kubuxu commented Jan 3, 2018

I think you can rule out bbloom as it is is only used in BloomCache in blockstore which is disabled by default. It is in critical path of every block request and every Has call.

@Kubuxu

This comment has been minimized.

Member

Kubuxu commented Jan 3, 2018

Updating gogo-protobuf is not trivial as it requires reintroduction of a bug fixed in that library: #3214

@Kubuxu

This comment has been minimized.

Member

Kubuxu commented Jan 3, 2018

Regarding FUSE, last time I looked the https://github.com/hanwen/go-fuse looked reasonable.

@Stebalien

This comment has been minimized.

Contributor

Stebalien commented Jan 3, 2018

Updating gogo-protobuf is not trivial as it requires reintroduction of a bug fixed in that library: #3214

For performance reasons, I'd like to update gogo protobuf for everything except pbnodes anyways (which we may be able to just parse manually).

Regarding FUSE, last time I looked the https://github.com/hanwen/go-fuse looked reasonable.

How hard do you think the switch will be? I have no experience with this area.

@Kubuxu

This comment has been minimized.

Member

Kubuxu commented Jan 31, 2018

How hard do you think the switch will be? I have no experience with this area.

From what I can recall it is structured differently from our current FUSE library. It might make sense to extract FUSE support out of go-ipfs if we decide to change the lib (we wanted to do that for a long time).

@djdv

This comment has been minimized.

Contributor

djdv commented Mar 27, 2018

@Kubuxu
I have plans to implement mounting on Windows, current options I am looking at are https://github.com/billziss-gh/winfsp and https://github.com/dokan-dev/dokany
Both offer a native approach and fuse wrappers.
https://github.com/billziss-gh/cgofuse
https://godoc.org/github.com/keybase/kbfs/dokan

I'm okay with using separate dependencies for separate platforms, but if a big move is planned anyway, it may be best to try and unify things in the process.

@Kubuxu

This comment has been minimized.

Member

Kubuxu commented Mar 27, 2018

@djdv friend of mine is figuring out FUSE support using CoreAPI as a plugin to go-ipfs (until we have transport for CoreAPI) and extract it later.

Also see: https://github.com/richardschneider/net-ipfs-mount


IMO mounting support currently is not that high priority (I think there are more pressing issues on Windows). I also think that even in case of Linux it is closer to PoC than a complete feature.

@djdv

This comment has been minimized.

Contributor

djdv commented Mar 27, 2018

FUSE support using CoreAPI as a plugin to go-ipfs

Neat.

net-ipfs-mount

I've tried this before but have had poor experiences with my own ipfs repo, it seems to work fine for small ones but with mine it seems to choke. It has been some time though, maybe it is better now, I will give it a go.

there are more pressing issues on Windows

Agreed, the intention is to think more on that later. When FUSE is being discussed I have to chime in though, I don't want Windows to be left behind. ;^)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment