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

Implemented WOLFSSH_FTP_FSETSTAT #551

Merged

Conversation

falemagn
Copy link
Contributor

Added support for the SSH_FXP_FSETSTAT packet type.

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@falemagn falemagn force-pushed the pull-reqs/cca01a4_WOLFSSH_FTP_FSETSTAT branch from 3e4ac58 to 3d6dfb3 Compare July 21, 2023 09:53
@JacobBarthelmeh JacobBarthelmeh self-assigned this Jul 21, 2023
@JacobBarthelmeh
Copy link
Contributor

ok to test

JacobBarthelmeh
JacobBarthelmeh previously approved these changes Aug 8, 2023
wolfssh/wolfsftp.h Outdated Show resolved Hide resolved
WLOG(WS_LOG_SFTP, "Receiving WOLFSSH_FTP_FSETSTAT");

/* get file handle */
ato32(data + idx, &sz); idx += UINT32_SZ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can maxSz be less than 4?

Copy link
Contributor Author

@falemagn falemagn Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you're referring to the line below the one you've commented on.

maxSz is a parameter of the function, and it's basically the size of the buffer, as far as I can tell. It's the same kind of code that is used throughout the wolfsftp.c file and, indeed, the code I suppose you're referring to has been copied and pasted from other functions that need to get the file handle, which predate this PR, like wolfSSH_SFTP_RecvClose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line I commented is pulling 4 bytes of data from the buffer.

If the maxSz is guaranteed to be more than 4 bytes, then you don't need a check to whether the buffer is big enough. Otherwise a check is needed to ensure there are 4 bytes to read.

Copy link
Contributor Author

@falemagn falemagn Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now. If that were an issue, then it would be an issue also in wolfSSH_SFTP_RecvClose and all other functions that do the same thing. I didn't delve into the code that deep, I just assumed that if it's correct elsewhere, then it's correct there, given that the way it gets there is the same as for all the other functions.

@JacobBarthelmeh
Copy link
Contributor

Some additional sanity checks have now been added in to wolfSSH master.

@JacobBarthelmeh JacobBarthelmeh removed their assignment Aug 31, 2023
@JacobBarthelmeh JacobBarthelmeh dismissed SparkiDev’s stale review September 15, 2023 21:42

I added in a sanity check

@ejohnstown ejohnstown merged commit 79bc747 into wolfSSL:master Sep 15, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants