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

broken sftp_quote logic #2143

Closed
bagder opened this issue Dec 2, 2017 · 3 comments
Closed

broken sftp_quote logic #2143

bagder opened this issue Dec 2, 2017 · 3 comments
Labels

Comments

@bagder
Copy link
Member

bagder commented Dec 2, 2017

in the beginning of the function lib/ssh-libssh.c:sftp_quote() there's this line:

char *cmd = sshc->quote_item->data;

This is fine, because we know sshc->quote_item to be non-NULL due to earlier checks before this function is called.

This cmd pointer is then dereferenced and possibly increased but on line 2528 it is checked for NULL?

if(cmd) {

... if cmd would be NULL there, it would already have done wrong and crashed. It can't be NULL there!

But the if condition always ends with a return; and below, outside of the if block (on line 2657 in my version - this means this is code that can never be reached: dead code), there's a final check if sshc->quote_item is NULL (which again, it can't be) and if it is, it would call state()...

(CID 1424902 by coverity, interpreted a bit by me)

/cc @nmav

@bagder bagder added the SCP/SFTP label Dec 2, 2017
@nmav
Copy link
Contributor

nmav commented Dec 3, 2017

That function code is very similar with the SSH_SFTP_QUOTE: block in the ssh.c state machine. The same behavior seems to be there too. The if null check seems indeed incorrect. However, whether cmd is null at the call of this function, I cannot really tell. It depends on whether a quote_item can contain null data

@nmav
Copy link
Contributor

nmav commented Dec 3, 2017

I think the coverity errors 1424908, 1424902 and 1424901 are also shared between the two back-ends, though the last looks like a false negative. That is pretty much an argument for @kdudka 's suggestion to make the SCP/SFTP part separated from the back-end.

@bagder
Copy link
Member Author

bagder commented Dec 4, 2017

However, whether cmd is null at the call of this function, I cannot really tell

But if it could, it still would be pointless to check that after it has already been dereferenced / increased!

And thanks for pointing out that this flaw exists in lib/ssh.c too. I'll take care of that in a PR!

bagder added a commit that referenced this issue Dec 4, 2017
Figured out while reviwring code in the libssh backend. The pointer was
checked for NULL after having been dereferenced, so we know it would
always equal true or it would've crashed.

Pointed-out-by: Nikos Mavrogiannopoulos

Bug #2143
bagder added a commit that referenced this issue Dec 4, 2017
Figured out while reviewing code in the libssh backend. The pointer was
checked for NULL after having been dereferenced, so we know it would
always equal true or it would've crashed.

Pointed-out-by: Nikos Mavrogiannopoulos

Bug #2143
Closes #2148
bagder added a commit that referenced this issue Dec 5, 2017
... by removing a superfluous NULL pointer check that also confuses
Coverity.

Fixes #2143
@bagder bagder closed this as completed in 85f0133 Dec 5, 2017
JohnDeHelian pushed a commit to JohnDeHelian/curl that referenced this issue Dec 7, 2017
... by removing a superfluous NULL pointer check that also confuses
Coverity.

Fixes curl#2143
Closes curl#2153
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants