fix(peer) String() bounds check #222
Conversation
| if len(pid) < maxRunes { | ||
| maxRunes = len(pid) | ||
| } | ||
| return "[Peer " + pid[:maxRunes] + "]" |
There was a problem hiding this comment.
argh, so much of the code assumes peers have valid multihash ids. we should either enforce those assumptions at the peer-creation interfaces, or everywhere else in the codebase.
There was a problem hiding this comment.
Is it generally safe to make certain assumptions about the length of multihashes?
There was a problem hiding this comment.
@maybebtc no, multihashes describe their own length, which is variable. you can safely assume ipfs peer.IDs to be longer than 64bits though.
|
not sure i want to go this route. i think we should instead enforce peers are generated correctly. ( |
|
Perhaps it might be safest to ensure correctness at ID generation and enforce this by only exposing functions that generate IDs safely. Then any peer.ID type found in the codebase is known to be correct. |
+1 |
|
@maybebtc status on this? |
|
IDs are far-reaching. Was thinking that we'd wait until commands gets merged. Do we want to prioritize the ID cleanup over commands? |
|
@maybebtc is this happening? otherwise I'll close it for now. |
|
I think we should merge this in as is for now. |
|
@maybebtc we should either validate it's a multihash or do nothing. the 12char check is not going to do much at all for us-- or is there some specific problem you want to work around? |
|
The length check prevents tests from failing when you attempt to run them verbosely. NB: This check is performed on the |
|
Ah somehow i misread it. ok, SGTM |
presently, fails
TODO ensure correctness at ID generation and enforce this by only exposing functions that generate IDs safely. Then any peer.ID type found in the codebase is known to be correct.
|
I'll rebase and merge |
jbenet
force-pushed
the
fix/peer-string-2014-10-28
branch
from
d3983d8
to
2e0371c
Compare
fixes #217