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

Don't limit the length of an input line in FTP #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nalind
Copy link

@nalind nalind commented Jul 30, 2013

This set of chages replaces the FTP client's use of static-sized "line" and "argbuf" buffers and fgets() to process command input with dynamic buffers and a knockoff of getline() that we'll call getinput().

It leaves the limit on the number of arguments unchanged, and makes a guess that macro expansion won't require more than 500 chars beyond the length of the macro definition and the current command, as it was entered.

Replace static buffers and fgets() with dynamic buffers and a knockoff
of getline() that we'll call getinput().  Leave the limit on the number
of arguments in place, and make a guess that macro expansion won't
require more than 500 chars beyond the length of the macro definition
and the current command as it was entered.
@greghudson
Copy link
Member

I'm not going to try to apply all of krb5's coding standards to ftpd because the current ftpd style is so far off, but there are some generic quality issues with this change:

  • getinput() has a complicated and undocumented contract. It would probably be less failure-prone to avoid trying to reuse the allocated buffer, and instead just have an output argument (or return value) for the allocated line which it read. In any event, the contract needs to be documented. I believe getinput() is used incorrectly by another() in ftp_cmds.c (tmp_len is not initialized by the caller), as evidence that the current contract is dangerous.
  • "tmp" is a bad variable name, especially when the variable is used for multiple purposes as in getinput(). The patch should use better variable names and should not reuse variables.

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.

None yet

2 participants