Skip to content

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.

2 participants