This repository has been archived by the owner on Nov 21, 2019. It is now read-only.
Do not cast a potentially non-String value to String, use ToString() instead. #117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Casting sounds wrong indeed. Is there a way to see this problem happening with a normal node build? |
|
I am sorry, I don't know how to reproduce this issue using a non-debug build. I tried to produce some test-case but I was not successful. On the other hand, I hope that you understand that this issue is worth fixing even without such a test-case. |
|
I understand. I will check the API docu tomorrow just to make sure, but will this pull in then. |
|
Did you find any problem while checking the API docs? Is there anything I can help with? |
|
@not-A-robot r+ |
|
📌 Commit 360c948 has been approved by |
|
⚡ Test exempted - status |
ghost
merged commit
ghost
pushed a commit
that referenced
this pull request
Apr 26, 2016
Do not cast a potentially non-String value to String, use ToString() instead. Note that Reader::createString() may not return String, it may return a Buffer (when return_buffers is set to true). So, the variable 'v' may not hold String. Moreover, 'v.As<String>()' doesn't convert the value to String, it works more like a cast. Of course, it is incorrect to cast Buffer to String. Unfortunately, the non-debug version of Node.js has various checks turned off. So, you cannot see this issue normally, but when you execute tests on the debug version of Node.js, i.e., run node_g test/reader.js then you can see the following crash caused by the incorrect cast of 'v'. ``` ==== C stack trace =============================== 1: V8_Fatal 2: v8::Utils::OpenHandle(v8::String const*, bool) 3: v8::Exception::Error(v8::Local<v8::String>) 4: 0x7efd7e262cb5 5: redisReaderGetReply 6: hiredis::Reader::Get(Nan::FunctionCallbackInfo<v8::Value> const&) 7: 0x7efd7e2626c6 8: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) 9: 0xdeaebe 10: 0xde59ae 11: 0xde5919 12: 0x1a1e1b8060bb Illegal instruction ```
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note that Reader::createString() may not return String, it may return a Buffer (when return_buffers is set to true). So, the variable 'v' may not hold String. Moreover, 'v.As()' doesn't convert the value to String, it works more like a cast. Of course, it is incorrect to cast Buffer to String. Unfortunately, the non-debug version of Node.js has various checks turned off. So, you cannot see this issue normally, but when you execute tests on the debug version of Node.js, i.e., run node_g test/reader.js then you can see the following crash caused by the incorrect cast of 'v'.