Fix an incoherent test. #4754
Fix an incoherent test. #4754
Conversation
richsalz
left a comment
There was a problem hiding this comment.
This will probably pick cleanly to 1.0.2
crypto/objects/obj_dat.c
Outdated
There was a problem hiding this comment.
How about adding {} to else according to "{} in both or neither". While you're at it...
There was a problem hiding this comment.
Well, there is another one at line 730. Well, given that it reduces chances for cherry-pick it either should be separate commit (if not request), or be ignored...
Unlikely, because it reads "return (num)" instead of "return num" in master. |
crypto/objects/obj_dat.c
Outdated
There was a problem hiding this comment.
Isn't check for s == NULL sufficient? At least if *o == '\0', then s will be NULL...
There was a problem hiding this comment.
Actually formally speaking this changes behaviour and question is if original behaviour, i.e. call to OBJ_create with s being NULL is intentional. If it is, then the suggested change would actually be justified... Or in other words have you examined if OBJ_create with s being NULL is bound to fail?
There was a problem hiding this comment.
If it is [intentional], then the suggested change would actually be justified...
I meant "unjustified".
There was a problem hiding this comment.
Well, given the fact that in case of s being NULL OBJ_create could be called with garbage l, it's probably appropriate to assume that at least that is a bug. Yet it doesn't actually lift all concerns about original behaviour. Original code was looping till empty string was returned by BIO_gets. While suggested modification will terminate the loop at first string that doesn't meet criteria...
There was a problem hiding this comment.
There is no error management in this corner of code, either from callers or into this called method ...
Should we silently ignore a invalid line in the middle of file ? or reject It ?
Suggestion : add code to count number of file line, and compare it to num (the oid creation counter) just before method return exit . so it may return -1 on error ?
There was a problem hiding this comment.
There is no error management in this corner of code
Which is kind of argument in favour of preserving the original behaviour at least in released branches, but probably even master [since it's meant to be minor release]. Please note that I don't have ready answer...
c26a3c4 to
03e6df7
Compare
crypto/objects/obj_dat.c
Outdated
There was a problem hiding this comment.
} else { is supposed to be one line.
crypto/objects/obj_dat.c
Outdated
crypto/objects/obj_dat.c
Outdated
There was a problem hiding this comment.
If we are deviating from original behaviour, then check for s == NULL is really sufficient...
There was a problem hiding this comment.
Today no; I keep it for a next PR ...
03e6df7 to
c43f691
Compare
|
Formally speaking this needs re-approval from original reviewers, but I suppose they can as well express it by simply committing... |
Pointer 'o' is set inside a local buffer, so it can't be NULL.
[extended tests]
c43f691 to
5671278
Compare
|
I'll do it now. |
Pointer 'o' is set inside a local buffer, so it can't be NULL. Also fix coding style and add comments Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4754)
|
Merged to master, 1.0.2, 1.1.0; thank you! |
|
Merged to all three branches as shown above; closing. |
Seen by PVS-Studio analysis in #4733
I guess the test is more wrong than duplicated ;)
Should I add a test on this method
OBJ_create_objects?Applicable to 1.1.0 and 1.0.2, backport ?
Checklist