Message ID | 1272003433-9426-1-git-send-email-scott.lovenberg@gmail.com |
---|---|
State | New |
Headers | show |
On Fri, 23 Apr 2010 02:17:12 -0400 Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > Remove magic numbers, redundant code and extra variables from open_cred_file(). > Remove check for domain length since strlcpy is safe from buffer overflows. > > Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com> > --- > mount.cifs.c | 78 +++++++++++++++++++++++---------------------------------- > 1 files changed, 32 insertions(+), 46 deletions(-) > > diff --git a/mount.cifs.c b/mount.cifs.c > index ba9e206..97dae82 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,13 +511,27 @@ toggle_dac_capability(int writable, int enable) > #endif /* HAVE_LIBCAP */ > #endif /* HAVE_LIBCAP_NG */ > > +/* > + * Null terminate 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) > @@ -541,50 +555,35 @@ 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 credentials 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_endl(line_buf); > > - /* NULL terminate at newline */ > - newline = strchr(line_buf + i, '\n'); > - if (newline) > - *newline = '\0'; > - > + /* parse user */ > 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) { > - fprintf(stderr, > - "mount.cifs failed due to malformed username in credentials file\n"); > - memset(line_buf, 0, 4096); > - return EX_USAGE; > - } > parsed_info->got_user = 1; > strlcpy(parsed_info->username, temp_val, > sizeof(parsed_info->username)); > } > - } 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; > @@ -592,7 +591,10 @@ static int open_cred_file(char *file_name, > i = set_password(parsed_info, temp_val); > 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 */ > @@ -600,22 +602,6 @@ static int open_cred_file(char *file_name, > if (parsed_info->verboseflag) > 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) { > - fprintf(stderr, > - "mount.cifs failed: domain in credentials file too long\n"); > - return EX_USAGE; > - } > - > strlcpy(parsed_info->domain, temp_val, > sizeof(parsed_info->domain)); > } > > --------------1.6.2.5-- > > > _______________________________________________ > linux-cifs-client mailing list > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client > Looks good. Committed with some minor whitespace cleanup.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Sat, 24 Apr 2010 19:21:08 -0400 Scott Lovenberg <scott.lovenberg@gmail.com> wrote: Maybe try not to put all of the description on the subject line. You can use multiple lines and it makes it a little tougher to import the patch. > Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com> > --- > mount.cifs.c | 141 ++++++++++++++++++++++++++++++++++------------------------ > 1 files changed, 83 insertions(+), 58 deletions(-) > > diff --git a/mount.cifs.c b/mount.cifs.c > index 97dae82..09d6351 100644 > --- a/mount.cifs.c > +++ b/mount.cifs.c > @@ -121,6 +121,14 @@ > */ > #define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV) > > +/* > + * Values for parsing a credentials file. > + */ > +#define CRED_UNPARSEABLE 0 > +#define CRED_USER 1 > +#define CRED_PASS 2 > +#define CRED_DOM 4 > + > /* struct for holding parsed mount info for use by privleged process */ > struct parsed_mount_info { > unsigned long flags; > @@ -511,20 +519,43 @@ toggle_dac_capability(int writable, int enable) > #endif /* HAVE_LIBCAP */ > #endif /* HAVE_LIBCAP_NG */ > > -/* > - * Null terminate string at first '\n' > - */ > -static void null_terminate_endl(char* source) > +static void null_terminate_endl(char *source) > { > - char* newline = strchr(source, '\n'); > + char *newline = strchr(source, '\n'); > if (newline) > *newline = '\0'; > } > > - > +/* > + * Parse a line from the credentials file. Changes target to first > + * character after '=' on 'line' and returns the value type of the line > + * Returns CRED_UNPARSEABLE on failure or if either parameter is NULL. > + */ > +static int parse_cred_line(char *line, char **target) > +{ > + if (line == NULL || target == NULL) > + goto parsing_err; > + > + /* position target at first char of value */ > + *target = strchr(line, '='); > + if (!target) ^^^^^^^^^^^^ Should be: if (!*target) ...right? > + goto parsing_err; > + *target += 1; > + > + /* tell the caller which value target points to */ > + if (strncasecmp("user", line, 4) == 0) > + return CRED_USER; > + if (strncasecmp("pass", line, 4) == 0) > + return CRED_PASS; > + if (strncasecmp("dom", line, 3) == 0) > + return CRED_DOM; > + > + parsing_err: > + return CRED_UNPARSEABLE; > +} > > static int open_cred_file(char *file_name, > - struct parsed_mount_info *parsed_info) > + struct parsed_mount_info *parsed_info) > { > char *line_buf; > char *temp_val; > @@ -535,30 +566,29 @@ static int open_cred_file(char *file_name, > > i = toggle_dac_capability(0, 1); > if (i) > - return i; > - > + goto return_i; > + > i = access(file_name, R_OK); > if (i) { > toggle_dac_capability(0, 0); > - return i; > + goto return_i; > } > > fs = fopen(file_name, "r"); > if (fs == NULL) { > toggle_dac_capability(0, 0); > - return errno; > + i = errno; > + goto return_i; > } > > i = toggle_dac_capability(0, 0); > - if (i) { > - fclose(fs); > - return i; > - } > - > - line_buf = (char *)malloc(line_buf_size); > + if(i) > + goto return_i; > + > + line_buf = (char*)malloc(line_buf_size); > if (line_buf == NULL) { > - fclose(fs); > - return EX_SYSERR; > + i = EX_SYSERR; > + goto return_i; > } > > /* parse line from credentials file */ > @@ -570,47 +600,42 @@ static int open_cred_file(char *file_name, > } > null_terminate_endl(line_buf); > > - /* parse user */ > - if (strncasecmp("user", line_buf + i, 4) == 0) { > - temp_val = strchr(line_buf + i, '='); > - if (temp_val) { > - /* go past equals sign */ > - temp_val++; > - parsed_info->got_user = 1; > - strlcpy(parsed_info->username, temp_val, > - sizeof(parsed_info->username)); > - } > - } > - > - /* parse password */ > - else if (strncasecmp("pass", line_buf + i, 4) == 0) { > - temp_val = strchr(line_buf + i, '='); > - if (!temp_val) > - continue; > - ++temp_val; > - i = set_password(parsed_info, temp_val); > - if (i) > - return i; > - } > - > - /* parse domain */ > - else if (strncasecmp("dom", line_buf + i, 3) == 0) { > - temp_val = strchr(line_buf + i, '='); > - if (temp_val) { > - /* go past equals sign */ > - temp_val++; > - if (parsed_info->verboseflag) > - fprintf(stderr, "\nDomain %s\n", > - temp_val); > - strlcpy(parsed_info->domain, temp_val, > - sizeof(parsed_info->domain)); > - } > + /* parse next token */ > + switch(parse_cred_line(line_buf + i, &temp_val)) { > + case CRED_USER: > + parse_username(temp_val, parsed_info); I think you should check the return value from parse_username here too. > + break; > + case CRED_PASS: > + if ((i = set_password(parsed_info, temp_val))) > + goto return_i; > + break; > + case CRED_DOM: > + if (parsed_info->verboseflag) > + fprintf(stderr, "\nDomain %s\n", > + temp_val); > + strlcpy(parsed_info->domain, temp_val, > + sizeof(parsed_info->domain)); > + break; > + case CRED_UNPARSEABLE: > + if (parsed_info->verboseflag) > + fprintf(stderr, > + "Credential formatted incorrectly: %s", > + temp_val); > + break; > } > - > } > - fclose(fs); > - SAFE_FREE(line_buf); > - return 0; > + > + i = 0; > + /* jump point for errors */ > + return_i: ^^^^^^^^ Minor nit: typically labels are flush to the left, and the code below shouldn't be indented as much. > + if (fs != NULL) > + fclose(fs); > + > + /* make sure passwords are scrubbed from memory */ > + if (line_buf != NULL) > + memset(line_buf, 0, line_buf_size); > + SAFE_FREE(line_buf); > + return i; > } > > static int There's also some trailing whitespace and other formatting errors that checkpatch complained about, and the patch didn't apply cleanly to the head git commit. I cleaned up most of these problems and will send a revised patch in a bit. - -- Jeff Layton <jlayton@samba.org> -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (GNU/Linux) iEYEARECAAYFAkvURfsACgkQyP0gxQMdzIAcNgCgkfVOVnYMIeLgxjnmDlFjURTV sCQAn2gmgmkJ80WDfc/UDwyY5uhqNbsJ =iFIZ -----END PGP SIGNATURE-----
diff --git a/mount.cifs.c b/mount.cifs.c index ba9e206..97dae82 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,13 +511,27 @@ toggle_dac_capability(int writable, int enable) #endif /* HAVE_LIBCAP */ #endif /* HAVE_LIBCAP_NG */ +/* + * Null terminate 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) @@ -541,50 +555,35 @@ 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 credentials 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_endl(line_buf); - /* NULL terminate at newline */ - newline = strchr(line_buf + i, '\n'); - if (newline) - *newline = '\0'; - + /* parse user */ 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) { - fprintf(stderr, - "mount.cifs failed due to malformed username in credentials file\n"); - memset(line_buf, 0, 4096); - return EX_USAGE; - } parsed_info->got_user = 1; strlcpy(parsed_info->username, temp_val, sizeof(parsed_info->username)); } - } 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; @@ -592,7 +591,10 @@ static int open_cred_file(char *file_name, i = set_password(parsed_info, temp_val); 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 */ @@ -600,22 +602,6 @@ static int open_cred_file(char *file_name, if (parsed_info->verboseflag) 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) { - fprintf(stderr, - "mount.cifs failed: domain in credentials file too long\n"); - return EX_USAGE; - } - strlcpy(parsed_info->domain, temp_val, sizeof(parsed_info->domain)); }
Remove magic numbers, redundant code and extra variables from open_cred_file(). Remove check for domain length since strlcpy is safe from buffer overflows. Signed-off-by: Scott Lovenberg <scott.lovenberg@gmail.com> --- mount.cifs.c | 78 +++++++++++++++++++++++---------------------------------- 1 files changed, 32 insertions(+), 46 deletions(-) --------------1.6.2.5--