Fix two bugs in basex decoder

keybase / saltpack

a modern crypto messaging format

https://saltpack.org/

BSD 3-Clause "New" or "Revised" License

989 stars 63 forks source link

Fix two bugs in basex decoder #32

Closed akalin-keybase closed 7 years ago

akalin-keybase commented 7 years ago

First bug: io.ReadAtLeast drops errors if it reads enough characters.

Second bug: both io.ReadAtLeast and punctuatedReader (among other) things return io.ErrUnexpectedEOF, so we were dropping errors from the latter.

The solution to both is to not use io.ReadAtLeast, and instead refill the buffer manually.

Reenable a test (which was hitting the second bug!).

Fix a few typos.

coveralls commented 7 years ago

Coverage decreased (-0.05%) to 86.174% when pulling 16070dd90667366a4ea0d25addef0972efbf8030 on akalin/fix-basex-bug into 8225c40ff652229f29123031877ae39259eed50f on master.

coveralls commented 7 years ago

Coverage decreased (-0.05%) to 86.174% when pulling f49cc821908b26bda0799158d9d2fed390d9fd87 on akalin/fix-basex-bug into 8225c40ff652229f29123031877ae39259eed50f on master.

akalin-keybase commented 7 years ago

Is there a way to see which particular lines became uncovered? I don't think I removed any important coverage...

oconnor663 commented 7 years ago

Looks like there is not :( https://github.com/lemurheavy/coveralls-public/issues/359

In general I'm much more concerned with decryption coverage than with anything else.

akalin-keybase commented 7 years ago

Yeah, I'm almost certain the coverage decrease was because I added a comment on top of an uncovered line (the TODO).

akalin-keybase commented 7 years ago

Looks like this bug is also present in go's standard library. Filed bug: https://github.com/golang/go/issues/20044

oconnor663 commented 7 years ago

Yeah I noticed their ReadFull function has the same behavior, because it calls ReadAtLeast on the inside.