https://bugzilla.mindrot.org/show_bug.cgi?id=2687
--- Comment #7 from Jakub Jelen <[email protected]> --- Thanks for comments and thoughtful reread. Adding answers where it makes sense. (In reply to Darren Tucker from comment #4) > Comment on attachment 2953 [details] > > response = read_passphrase("Accept updated hostkeys? " > > "(yes/no): ", RP_ECHO); > >- if (strcasecmp(response, "yes") == 0) > >+ if (response != NULL && strcasecmp(response, "yes") == > >0) > > I think this is a false positive. > read_passphrase() can only return NULL if given the RP_ALLOW_EOF > flag, otherwise the return values all come from xstrdup which will > provide a valid pointer or die trying. In that case it does not make sense to compare with NULL two lines below (which was the concern of coverity report REVERSE_INULL): > else if (quit_pending || response == NULL || yes. My patch was one possible solution, but there are other (maybe better) ways how to make it clear. > > dump_cfg_string(ServerOpCodes code, const char *val) > > { > >- if (val == NULL) > >- return; > > printf("%s %s\n", lookup_opcode_name(code), > > val == NULL ? "none" : val); > > not sure what the intent of this was, will need to investigate. either the first or the second check for NULL is bogus. We should prefer dumping configuration options with "none" rather than not, but making sure that the options have empty value "none" accepted is a good thing to consider before applying. (In reply to Darren Tucker from comment #6) > Comment on attachment 2954 [details] > 2nd part with lower priority > >- filename, linenum, arg ? arg : "<NONE>"); > >+ filename, linenum, arg); > > I'm not sure removing this is a good idea; it might not be possible > for arg to be null right now but later? That is valid point. But now it is just a bogus check. I don't think these days any of the options allow empty argument (and only case where I left the <NONE> check was in (s|o)LogLevel and sLogFacility, which do not have the check for missing argument in the first place. It quite looked like copy&paste "error" from some other places. Standardizing also the other mentioned case to test for missing argument in first place would probably make sense too. -- You are receiving this mail because: You are watching the assignee of the bug. You are watching someone on the CC list of the bug. _______________________________________________ openssh-bugs mailing list [email protected] https://lists.mindrot.org/mailman/listinfo/openssh-bugs
