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

fix overflow in older android #81

Merged
merged 3 commits into from Oct 6, 2015
Merged

Conversation

calvinmetcalf
Copy link
Contributor

if the tests take too long I can set up what we do for readable-stream where each browser is it's own saucelabs test

edit by @dcousens: see #81 (comment) for the implications of this PR

@calvinmetcalf calvinmetcalf mentioned this pull request Oct 6, 2015
@calvinmetcalf
Copy link
Contributor Author

FOUND A FAILING TEST!

@@ -114,3 +114,18 @@ test('hex of write{Uint,Int}{8,16,32}{LE,BE} with overflow', function (t) {
}
t.end()
})
test('android issue', function (t) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe name this test "Can read and write Int32BE (ref #80)" or some relevant issue so its clear this will be a regression test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still sussing out the root issue will totally give it a decent test name once it's all ready

@calvinmetcalf calvinmetcalf changed the title test against older android fix overflow in older android Oct 6, 2015
@calvinmetcalf
Copy link
Contributor Author

ok figured it out and this pull now contains the fix, basically older android wouldn't properly coerce the value into a 8 bit number so we have to add some masking to make sure they do it right

@dcousens
Copy link
Collaborator

dcousens commented Oct 6, 2015

Hahaha, damn. I should have mentioned that when I re-read through the buffer code for this bug.
The assignment to the typed array was about the only problem I could forsee might have a problem with an overflow, but I had no way to test it quickly.

By chance does it work if you | 0 asm.js it?
I get the feeling | 0 will work because the above integers are fine.

@calvinmetcalf
Copy link
Contributor Author

I don't think so, the error showed up with sha1 hashes where you where doing that before writing

dcousens added a commit that referenced this pull request Oct 6, 2015
@dcousens dcousens merged commit 9d787e5 into feross:master Oct 6, 2015
@dcousens dcousens added the bug label Oct 6, 2015
@dcousens dcousens self-assigned this Oct 6, 2015
@nolanlawson
Copy link
Collaborator

Nice catch!

@calvinmetcalf
Copy link
Contributor Author

(though | could be wrong, the main reason I picked & 0xff was because it was used other places in the code)

@calvinmetcalf calvinmetcalf deleted the older-android branch October 6, 2015 23:36
@dcousens
Copy link
Collaborator

dcousens commented Oct 6, 2015

@calvinmetcalf are you able to test the | 0 idea on your emulator?
Might give us a better understanding of the bug such that we can check other parts of the code for potential problems.

@dcousens
Copy link
Collaborator

dcousens commented Oct 6, 2015

@feross ready to publish

@feross
Copy link
Owner

feross commented Oct 6, 2015

Thanks for the good work guys – just running tests once more and will publish.

@feross
Copy link
Owner

feross commented Oct 7, 2015

Published as 3.5.1.

@rubensayshi
Copy link

so now browserify needs to bump it's version of buffer from ^3.4.3 to ^3.5.1 ?

@dcousens
Copy link
Collaborator

dcousens commented Oct 7, 2015

@rubensayshi that shouldn't be necessary due to the ^.

@dcousens
Copy link
Collaborator

dcousens commented Oct 7, 2015

@FiloSottile @Sidnicious @aianus @ajp8164 @barmstrong @bechi @bitjson @bpdavenport @braydonf @caedesvvv @chjj @cmgustavo @coblee @defunctzombie @dionyziz @dominictarr @fanatid @gabegattis @gasteve @gentrysherrill @greenaddress @hudon @ionux @jprichardson @jsorchik @kyledrake @lclc @lord @maksim-s @maraoz @matiu @mihar @pnagurny @robby-dermody @rubensayshi @ryanxcharles @sembrestels @tgerring @thallium205 @unusualbob @vbuterin @weilu @williamcotton @zQueal @zootreeves

Please all acknowledge the above bug.

This bug affects any use of typed arrays in Android 4.3 [and lower] native web views.

If you were using browserify and Buffer (which opts for a typed array if available), and you are targeting Android <=4.3 platform in a native web view, your users are going to suffer a potential loss of funds.

This bug is now avoided by the above PR, but if this bug is as serious as it looks, it is likely an overflowing assignment may occur elsewhere we haven't found yet.
If you are developing mobile applications [on Android, in javascript], you can use https://crosswalk-project.org/ to ensure a consistent web view (understand the implications of that before deploying though).

Sorry for tagging so many of you, but short of sending out multiple emails and tracking down all your email addresses, this seemed the most effective way of reaching out.
Please tag any other developers who may be affected by this problem.

@matiu
Copy link

matiu commented Oct 8, 2015

thanks a lot for the notification Daniel.

On Wed, Oct 7, 2015 at 9:00 PM, Daniel Cousens notifications@github.com
wrote:

@FiloSottile https://github.com/FiloSottile @Sidnicious
https://github.com/Sidnicious @aianus https://github.com/aianus
@ajp8164 https://github.com/ajp8164 @barmstrong
https://github.com/barmstrong @bechi https://github.com/bechi @bitjson
https://github.com/bitjson @bpdavenport https://github.com/bpdavenport
@braydonf https://github.com/braydonf @caedesvvv
https://github.com/caedesvvv @chjj https://github.com/chjj @cmgustavo
https://github.com/cmgustavo @coblee https://github.com/coblee
@defunctzombie https://github.com/defunctzombi%20e @dionyziz
https://github.com/dionyziz @dominictarr
https://github.com/dominictarr @fanatid https://github.com/fanatid
@gabegattis https://github.com/gabegattis @gasteve
https://github.com/gasteve @gentrysherrill
https://github.com/gentrysherrill @greenaddress
https://github.com/greenaddress @hudon https://github.com/hudon @ionux
https://github.com/ionux @jprichardson https://github.com/jprichardson
@jsorchik https://github.com/jsorchik @kyledrake
https://github.com/kyledrake @lclc https://github.com/lclc@lord
https://github.com/lord @maksim-s https://github.com/maksim-s @maraoz
https://github.com/maraoz @matiu https://github.com/matiu @mihar
https://github.com/mihar @pnagurny https://github.com/pnagurny
@robby-dermody https://github.com/robby-dermody @rubensayshi
https://github.com/rubensayshi @ryanxcharles
https://github.com/ryanxcharles @sembrestels
https://github.com/sembrestels @tgerring https://github.com/tgerring
@thallium205 https://github.com/thallium205 @unusualbob
https://github.com/unusualbob @vbuterin @weilu
https://github.com/weilu @williamcotton
https://github.com/williamcotton @zQueal https://github.com/zQueal
@zootreeves https://github.com/zootreeves


Reply to this email directly or view it on GitHub
#81 (comment).

Matías Alejo Garcia
@EMATIU
Roads? Where we're going, we don't need roads!

@rubensayshi
Copy link

for anyone who failed to grasp where this extends into, crypto (and thus create-hash and create-hmac) packages are affected by this, giving incorrect hashes in some cases.
this extends for example into bitcoin related things; you'll have incorrect BIP32 derivations and incorrect addresses (which is how I found out about this).

@levino
Copy link

levino commented Oct 10, 2015

@flotob @meinharrd @chmac

rubensayshi added a commit to blocktrail/blocktrail-sdk-nodejs that referenced this pull request Oct 21, 2015
this is build with a browserify fix that affected android 4.3 and lower;
feross/buffer#81
rubensayshi added a commit to blocktrail/blocktrail-sdk-nodejs that referenced this pull request Oct 21, 2015
this is build with a browserify fix that affected android 4.3 and lower;
feross/buffer#81
rubensayshi added a commit to blocktrail/blocktrail-sdk-nodejs that referenced this pull request Oct 21, 2015
this is build with a browserify fix that affected android 4.3 and lower;
feross/buffer#81
rubensayshi added a commit to blocktrail/blocktrail-sdk-nodejs that referenced this pull request Oct 21, 2015
this is build with a browserify fix that affected android 4.3 and lower;
feross/buffer#81
rubensayshi added a commit to blocktrail/blocktrail-sdk-nodejs that referenced this pull request Oct 21, 2015
this is build with a browserify fix that affected android 4.3 and lower;
feross/buffer#81
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants