crypto: have fixed NodeBIOs return EOF. #5105
Conversation
Fishrock123
added
crypto
|
cc @nodejs/crypto |
agl
changed the title
|
Is it possible/reasonable to write a JS-land test for this change? |
| @@ -27,6 +28,21 @@ BIO* NodeBIO::New() { | |||
| } | |||
|
|
|||
|
|
|||
| BIO* NodeBIO::NewFixed(const char *data, size_t len) { | |||
There was a problem hiding this comment.
Nit, but let's use const char* data here. This is how it is in the rest of the file.
|
@agl thank you so much for this PR. Just curious, what would be needed to be done if we were using |
|
One nit, otherwise LGTM |
|
cc @bnoordhuis |
|
@Trott I don't think that there is any user-facing JS property or function that exposes EOF behavior of BIO buffers. |
|
LGTM sans nits that Fedor pointed out. CI: https://ci.nodejs.org/job/node-test-pull-request/1589/ |
Prior to this change, the NodeBIO objects used to wrap fixed data had `num` equal to -1. This caused them to return -1 and set the retry flags when they ran out of data. Since the data is fixed, that's incorrect. Instead they should return zero to signal EOF. This change adds a new, static function, NodeBIO::NewFixed to create a BIO that wraps fixed data and which returns zero when exhausted. The practical impact of this is limited since most (all?) the parsing functions that these BIOs get passed to consider any return value less than one to be EOF and ignore the retry flags anyway.
OpenSSL does provide You can see it also does effectively the same thing as this code by setting Using a plain |
|
Ok, thank you very much for explanation @agl ! |
I did look into that and it would be possible to test this if I could find a case that pushed a different value on the error queue when the BIO returned premature EOF vs signaling an error. Somewhere in OpenSSL there is probably something that does that, but I couldn't easily find it in the code paths that node.js is using. |
|
LGTM |
|
(I amended the change, because I'm used to Gerrit, but that seems to have blown away the line comments. Sorry if that was the wrong thing to do! I believe that I address them all.) |
|
@agl that's fine, we can still view them by clicking on them in this thread. |
|
Landed in 773b901, thank you! |
Prior to this change, the NodeBIO objects used to wrap fixed data had `num` equal to -1. This caused them to return -1 and set the retry flags when they ran out of data. Since the data is fixed, that's incorrect. Instead they should return zero to signal EOF. This change adds a new, static function, NodeBIO::NewFixed to create a BIO that wraps fixed data and which returns zero when exhausted. The practical impact of this is limited since most (all?) the parsing functions that these BIOs get passed to consider any return value less than one to be EOF and ignore the retry flags anyway. PR-URL: #5105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Prior to this change, the NodeBIO objects used to wrap fixed data had `num` equal to -1. This caused them to return -1 and set the retry flags when they ran out of data. Since the data is fixed, that's incorrect. Instead they should return zero to signal EOF. This change adds a new, static function, NodeBIO::NewFixed to create a BIO that wraps fixed data and which returns zero when exhausted. The practical impact of this is limited since most (all?) the parsing functions that these BIOs get passed to consider any return value less than one to be EOF and ignore the retry flags anyway. PR-URL: #5105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Prior to this change, the NodeBIO objects used to wrap fixed data had `num` equal to -1. This caused them to return -1 and set the retry flags when they ran out of data. Since the data is fixed, that's incorrect. Instead they should return zero to signal EOF. This change adds a new, static function, NodeBIO::NewFixed to create a BIO that wraps fixed data and which returns zero when exhausted. The practical impact of this is limited since most (all?) the parsing functions that these BIOs get passed to consider any return value less than one to be EOF and ignore the retry flags anyway. PR-URL: nodejs#5105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Prior to this change, the NodeBIO objects used to wrap fixed data had `num` equal to -1. This caused them to return -1 and set the retry flags when they ran out of data. Since the data is fixed, that's incorrect. Instead they should return zero to signal EOF. This change adds a new, static function, NodeBIO::NewFixed to create a BIO that wraps fixed data and which returns zero when exhausted. The practical impact of this is limited since most (all?) the parsing functions that these BIOs get passed to consider any return value less than one to be EOF and ignore the retry flags anyway. PR-URL: #5105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Prior to this change, the NodeBIO objects used to wrap fixed data had `num` equal to -1. This caused them to return -1 and set the retry flags when they ran out of data. Since the data is fixed, that's incorrect. Instead they should return zero to signal EOF. This change adds a new, static function, NodeBIO::NewFixed to create a BIO that wraps fixed data and which returns zero when exhausted. The practical impact of this is limited since most (all?) the parsing functions that these BIOs get passed to consider any return value less than one to be EOF and ignore the retry flags anyway. PR-URL: #5105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Prior to this change, the NodeBIO objects used to wrap fixed data had `num` equal to -1. This caused them to return -1 and set the retry flags when they ran out of data. Since the data is fixed, that's incorrect. Instead they should return zero to signal EOF. This change adds a new, static function, NodeBIO::NewFixed to create a BIO that wraps fixed data and which returns zero when exhausted. The practical impact of this is limited since most (all?) the parsing functions that these BIOs get passed to consider any return value less than one to be EOF and ignore the retry flags anyway. PR-URL: #5105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Prior to this change, the NodeBIO objects used to wrap fixed data had
numequal to -1. This caused them to return -1 and set the retry flagswhen they ran out of data. Since the data is fixed, that's incorrect.
Instead they should return zero to signal EOF.
This change adds a new, static function, NodeBIO::NewFixed to create a
BIO that wraps fixed data and which returns zero when exhausted.
The practical impact of this is limited since most (all?) the parsing
functions that these BIOs get passed to consider any return value less
than one to be EOF and ignore the retry flags anyway.