<br><br><div class="gmail_quote">On Sun, Apr 25, 2010 at 9:39 AM, Jeff Layton <span dir="ltr"><<a href="mailto:jlayton@samba.org">jlayton@samba.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
-----BEGIN PGP SIGNED MESSAGE-----<br>
Hash: SHA1<br>
<br>
On Sat, 24 Apr 2010 19:21:08 -0400<br>
Scott Lovenberg <<a href="mailto:scott.lovenberg@gmail.com">scott.lovenberg@gmail.com</a>> wrote:<br>
<br>
Maybe try not to put all of the description on the subject line. You<br>
can use multiple lines and it makes it a little tougher to import the<br>
patch.<br></blockquote><div>Hrm... those were multiple lines. There was a hard return after the first sentence.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5"><br>
> Signed-off-by: Scott Lovenberg <<a href="mailto:scott.lovenberg@gmail.com">scott.lovenberg@gmail.com</a>><br>
> ---<br>
> mount.cifs.c | 141 ++++++++++++++++++++++++++++++++++------------------------<br>
> 1 files changed, 83 insertions(+), 58 deletions(-)<br>
><br>
> diff --git a/mount.cifs.c b/mount.cifs.c<br>
> index 97dae82..09d6351 100644<br>
> --- a/mount.cifs.c<br>
> +++ b/mount.cifs.c<br>
> @@ -121,6 +121,14 @@<br>
> */<br>
> #define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV)<br>
><br>
> +/*<br>
> + * Values for parsing a credentials file.<br>
> + */<br>
> +#define CRED_UNPARSEABLE 0<br>
> +#define CRED_USER 1<br>
> +#define CRED_PASS 2<br>
> +#define CRED_DOM 4<br>
> +<br>
> /* struct for holding parsed mount info for use by privleged process */<br>
> struct parsed_mount_info {<br>
> unsigned long flags;<br>
> @@ -511,20 +519,43 @@ toggle_dac_capability(int writable, int enable)<br>
> #endif /* HAVE_LIBCAP */<br>
> #endif /* HAVE_LIBCAP_NG */<br>
><br>
> -/*<br>
> - * Null terminate string at first '\n'<br>
> - */<br>
> -static void null_terminate_endl(char* source)<br>
> +static void null_terminate_endl(char *source)<br>
> {<br>
> - char* newline = strchr(source, '\n');<br>
> + char *newline = strchr(source, '\n');<br>
> if (newline)<br>
> *newline = '\0';<br>
> }<br>
><br>
> -<br>
> +/*<br>
> + * Parse a line from the credentials file. Changes target to first<br>
> + * character after '=' on 'line' and returns the value type of the line<br>
> + * Returns CRED_UNPARSEABLE on failure or if either parameter is NULL.<br>
> + */<br>
> +static int parse_cred_line(char *line, char **target)<br>
> +{<br>
> + if (line == NULL || target == NULL)<br>
> + goto parsing_err;<br>
> +<br>
> + /* position target at first char of value */<br>
> + *target = strchr(line, '=');<br>
> + if (!target)<br>
</div></div> ^^^^^^^^^^^^<br>
Should be:<br>
if (!*target)<br>
<br>
...right?</blockquote><div>Correct. Silly mistake.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div><div class="h5">
> + goto parsing_err;<br>
> + *target += 1;<br>
> +<br>
> + /* tell the caller which value target points to */<br>
> + if (strncasecmp("user", line, 4) == 0)<br>
> + return CRED_USER;<br>
> + if (strncasecmp("pass", line, 4) == 0)<br>
> + return CRED_PASS;<br>
> + if (strncasecmp("dom", line, 3) == 0)<br>
> + return CRED_DOM;<br>
> +<br>
> + parsing_err:<br>
> + return CRED_UNPARSEABLE;<br>
> +}<br>
><br>
> static int open_cred_file(char *file_name,<br>
> - struct parsed_mount_info *parsed_info)<br>
> + struct parsed_mount_info *parsed_info)<br>
> {<br>
> char *line_buf;<br>
> char *temp_val;<br>
> @@ -535,30 +566,29 @@ static int open_cred_file(char *file_name,<br>
><br>
> i = toggle_dac_capability(0, 1);<br>
> if (i)<br>
> - return i;<br>
> -<br>
> + goto return_i;<br>
> +<br>
> i = access(file_name, R_OK);<br>
> if (i) {<br>
> toggle_dac_capability(0, 0);<br>
> - return i;<br>
> + goto return_i;<br>
> }<br>
><br>
> fs = fopen(file_name, "r");<br>
> if (fs == NULL) {<br>
> toggle_dac_capability(0, 0);<br>
> - return errno;<br>
> + i = errno;<br>
> + goto return_i;<br>
> }<br>
><br>
> i = toggle_dac_capability(0, 0);<br>
> - if (i) {<br>
> - fclose(fs);<br>
> - return i;<br>
> - }<br>
> -<br>
> - line_buf = (char *)malloc(line_buf_size);<br>
> + if(i)<br>
> + goto return_i;<br>
> +<br>
> + line_buf = (char*)malloc(line_buf_size);<br>
> if (line_buf == NULL) {<br>
> - fclose(fs);<br>
> - return EX_SYSERR;<br>
> + i = EX_SYSERR;<br>
> + goto return_i;<br>
> }<br>
><br>
> /* parse line from credentials file */<br>
> @@ -570,47 +600,42 @@ static int open_cred_file(char *file_name,<br>
> }<br>
> null_terminate_endl(line_buf);<br>
><br>
> - /* parse user */<br>
> - if (strncasecmp("user", line_buf + i, 4) == 0) {<br>
> - temp_val = strchr(line_buf + i, '=');<br>
> - if (temp_val) {<br>
> - /* go past equals sign */<br>
> - temp_val++;<br>
> - parsed_info->got_user = 1;<br>
> - strlcpy(parsed_info->username, temp_val,<br>
> - sizeof(parsed_info->username));<br>
> - }<br>
> - }<br>
> -<br>
> - /* parse password */<br>
> - else if (strncasecmp("pass", line_buf + i, 4) == 0) {<br>
> - temp_val = strchr(line_buf + i, '=');<br>
> - if (!temp_val)<br>
> - continue;<br>
> - ++temp_val;<br>
> - i = set_password(parsed_info, temp_val);<br>
> - if (i)<br>
> - return i;<br>
> - }<br>
> -<br>
> - /* parse domain */<br>
> - else if (strncasecmp("dom", line_buf + i, 3) == 0) {<br>
> - temp_val = strchr(line_buf + i, '=');<br>
> - if (temp_val) {<br>
> - /* go past equals sign */<br>
> - temp_val++;<br>
> - if (parsed_info->verboseflag)<br>
> - fprintf(stderr, "\nDomain %s\n",<br>
> - temp_val);<br>
> - strlcpy(parsed_info->domain, temp_val,<br>
> - sizeof(parsed_info->domain));<br>
> - }<br>
> + /* parse next token */<br>
> + switch(parse_cred_line(line_buf + i, &temp_val)) {<br>
> + case CRED_USER:<br>
> + parse_username(temp_val, parsed_info);<br>
<br>
</div></div>I think you should check the return value from parse_username here too.<br>
<div class="im"><br></div></blockquote><div>Probably a good idea. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">
> + break;<br>
> + case CRED_PASS:<br>
> + if ((i = set_password(parsed_info, temp_val)))<br>
> + goto return_i;<br>
> + break;<br>
> + case CRED_DOM:<br>
> + if (parsed_info->verboseflag)<br>
> + fprintf(stderr, "\nDomain %s\n",<br>
> + temp_val);<br>
> + strlcpy(parsed_info->domain, temp_val,<br>
> + sizeof(parsed_info->domain));<br>
> + break;<br>
> + case CRED_UNPARSEABLE:<br>
> + if (parsed_info->verboseflag)<br>
> + fprintf(stderr,<br>
> + "Credential formatted incorrectly: %s",<br>
> + temp_val);<br>
> + break;<br>
> }<br>
> -<br>
> }<br>
> - fclose(fs);<br>
> - SAFE_FREE(line_buf);<br>
> - return 0;<br>
> +<br>
> + i = 0;<br>
> + /* jump point for errors */<br>
> + return_i:<br>
<br>
</div> ^^^^^^^^<br>
Minor nit: typically labels are flush to the left, and the code below<br>
shouldn't be indented as much.<br>
<div class="im"><br>
> + if (fs != NULL)<br>
> + fclose(fs);<br>
> +<br>
> + /* make sure passwords are scrubbed from memory */<br>
> + if (line_buf != NULL)<br>
> + memset(line_buf, 0, line_buf_size);<br>
> + SAFE_FREE(line_buf);<br>
> + return i;<br>
> }<br>
><br>
> static int<br>
<br>
</div>There's also some trailing whitespace and other formatting errors that<br>
checkpatch complained about, and the patch didn't apply cleanly to the<br>
head git commit. I cleaned up most of these problems and will send a<br>
revised patch in a bit.<br>
<br></blockquote><div>Thanks, I forgot to synchronize with remote before I created the patch. As for the white space, I think that's an artifact from git, there wasn't any in the original patch. Still getting to the bottom of that. </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
- --<br>
<div class="im">Jeff Layton <<a href="mailto:jlayton@samba.org">jlayton@samba.org</a>><br>
</div>-----BEGIN PGP SIGNATURE-----<br>
Version: GnuPG v2.0.14 (GNU/Linux)<br>
<br>
iEYEARECAAYFAkvURfsACgkQyP0gxQMdzIAcNgCgkfVOVnYMIeLgxjnmDlFjURTV<br>
sCQAn2gmgmkJ80WDfc/UDwyY5uhqNbsJ<br>
=iFIZ<br>
-----END PGP SIGNATURE-----<br>
</blockquote></div><br><br clear="all"><br>-- <br>Peace and Blessings,<br>-Scott.<br><br>