[linux-cifs-client] [Patch] Clean up open_cred_file()
Jeff Layton
jlayton at samba.org
Sun Apr 18 05:26:02 MDT 2010
On Sun, 18 Apr 2010 03:57:31 -0400
Scott Lovenberg <scott.lovenberg at gmail.com> wrote:
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 1aa3329..54feeaf 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -98,7 +98,7 @@
> #endif
>
> #define MOUNT_PASSWD_SIZE 128
> -#define DOMAIN_SIZE 64
> +#define MAX_DOMAIN_SIZE 64
>
> /*
> * value of the ver= option that gets passed to the kernel. Used to indicate
> @@ -128,7 +128,7 @@ struct parsed_mount_info {
> char share[MAX_SHARE_LEN + 1];
> char prefix[PATH_MAX + 1];
> char options[MAX_OPTIONS_LEN];
> - char domain[DOMAIN_SIZE + 1];
> + char domain[MAX_DOMAIN_SIZE + 1];
> char username[MAX_USERNAME_SIZE + 1];
> char password[MOUNT_PASSWD_SIZE + 1];
> char addrlist[MAX_ADDR_LIST_LEN];
> @@ -511,14 +511,26 @@ toggle_dac_capability(int writable, int enable)
> #endif /* HAVE_LIBCAP */
> #endif /* HAVE_LIBCAP_NG */
>
> +/*
> + * Null terminates string at first '\n'.
> + */
> +static void null_terminate_endl(char* source)
> +{
> + char* newline = strchr(source, '\n');
> + if (newline)
> + *newline = '\0';
> +}
> +
> static int open_cred_file(char *file_name,
> struct parsed_mount_info *parsed_info)
> {
> char *line_buf;
> - char *temp_val, *newline;
> + char *temp_val;
> FILE *fs = NULL;
> - int i, length;
> -
> + int i;
> + const int line_buf_size = 4096;
> + const int min_non_white = 10;
> +
> i = toggle_dac_capability(0, 1);
> if (i)
> return i;
> @@ -541,58 +553,54 @@ static int open_cred_file(char *file_name,
> return i;
> }
>
> - line_buf = (char *)malloc(4096);
> + line_buf = (char *)malloc(line_buf_size);
> if (line_buf == NULL) {
> fclose(fs);
> return EX_SYSERR;
> }
>
> - while (fgets(line_buf, 4096, fs)) {
> - /* parse line from credential file */
> -
> + /* parse line from credential file*/
> + while (fgets(line_buf, line_buf_size, fs)) {
> /* eat leading white space */
> - for (i = 0; i < 4086; i++) {
> + for (i = 0; i < line_buf_size - min_non_white + 1; i++) {
> if ((line_buf[i] != ' ') && (line_buf[i] != '\t'))
> break;
> - /* if whitespace - skip past it */
> }
>
> - /* NULL terminate at newline */
> - newline = strchr(line_buf + i, '\n');
> - if (newline)
> - *newline = '\0';
> -
> + /* parse user */
> + null_terminate_endl(line_buf);
> if (strncasecmp("user", line_buf + i, 4) == 0) {
> temp_val = strchr(line_buf + i, '=');
> if (temp_val) {
> /* go past equals sign */
> temp_val++;
> - for (length = 0; length < 4087; length++) {
> - if ((temp_val[length] == '\n')
> - || (temp_val[length] == '\0')) {
> - temp_val[length] = '\0';
> - break;
> - }
> - }
> - if (length > 4086) {
> + null_terminate_endl(temp_val);
^^^ is this needed? You've already
wiped out the newline in the earlier
call to this.
> + if (strlen(temp_val) > MAX_USERNAME_SIZE) {
^^^^^^
Wonder if we really need this check?
parse_username shouldn't allow it to overflow
the buffer. It won't return an error -- it'll
just put what it can in the buffer, but
presumably the kernel would reject the
malformed username on the mount attempt.
> fprintf(stderr,
> "mount.cifs failed due to malformed username in credentials file\n");
> - memset(line_buf, 0, 4096);
> + memset(line_buf, 0, line_buf_size);
> return EX_USAGE;
> }
> - parsed_info->got_user = 1;
> - strlcpy(parsed_info->username, temp_val,
> - sizeof(parsed_info->username));
> + parse_username(temp_val, parsed_info);
You should probably also zero out the buffer here in case the
user= field held a password too.
> }
> - } else if (strncasecmp("pass", line_buf + i, 4) == 0) {
> + }
> +
> + /* parse password */
> + else if (strncasecmp("pass", line_buf + i, 4) == 0) {
> temp_val = strchr(line_buf + i, '=');
> if (!temp_val)
> continue;
> - ++temp_val;
> + /* go past equals sign */
> + temp_val++;
> i = set_password(parsed_info, temp_val);
> + /* zero out password from buffer */
> + memset(line_buf, 0, line_buf_size);
> if (i)
> return i;
> - } else if (strncasecmp("dom", line_buf + i, 3) == 0) {
> + }
> +
> + /* parse domain */
> + else if (strncasecmp("dom", line_buf + i, 3) == 0) {
> temp_val = strchr(line_buf + i, '=');
> if (temp_val) {
> /* go past equals sign */
> @@ -601,16 +609,8 @@ static int open_cred_file(char *file_name,
> fprintf(stderr, "\nDomain %s\n",
> temp_val);
>
> - for (length = 0; length < DOMAIN_SIZE + 1;
> - length++) {
> - if ((temp_val[length] == '\n')
> - || (temp_val[length] == '\0')) {
> - temp_val[length] = '\0';
> - break;
> - }
> - }
> -
> - if (length > DOMAIN_SIZE) {
> + null_terminate_endl(temp_val);
^^^ I don't think this is needed.
> + if (strlen(temp_val) > MAX_DOMAIN_SIZE) {
> fprintf(stderr,
> "mount.cifs failed: domain in credentials file too long\n");
> return EX_USAGE;
> @@ -620,7 +620,6 @@ static int open_cred_file(char *file_name,
> sizeof(parsed_info->domain));
> }
> }
> -
> }
> fclose(fs);
> SAFE_FREE(line_buf);
>
Other than the stuff above, the patch looks reasonable.
--
Jeff Layton <jlayton at samba.org>
More information about the linux-cifs-client
mailing list