Re: [PATCH][SMB3] mount.cifs: use SUDO_UID env variable for cruid

["Aurélien Aptel" <aaptel@suse.com>]

Shyam Prasad N <nspmangalore@gmail.com> writes:
> This is a fix for the scenario of a krb5 user running a "sudo mount".
> Even if the user has cred cache populated, when the mount is run using
> sudo, uid switches to 0. So cred cache for the root user will be
> searched for, unless cruid is specified explicitly. This fix checks
> for cruid=$SUDO_UID as a fallback option, when the mount fails with
> ENOKEY.

The idea seems good.

> @@ -2053,7 +2066,24 @@ int main(int argc, char **argv)
>  		parsed_info = NULL;
>  		fprintf(stderr, "Unable to allocate memory: %s\n",
>  				strerror(errno));
> -		return EX_SYSERR;
> +		rc = EX_SYSERR;
> +		goto mount_exit;
> +	}
> +
> +	reinit_parsed_info = 
> +		(struct parsed_mount_info *) malloc(sizeof(*reinit_parsed_info));
> +	if (reinit_parsed_info == NULL) {
> +		fprintf(stderr, "Unable to allocate memory: %s\n",
> +				strerror(errno));
> +		rc = EX_SYSERR;
> +		goto mount_exit;
> +	}
> +
> +	options = calloc(options_size, 1);
> +	if (!options) {
> +		fprintf(stderr, "Unable to allocate memory.\n");
> +		rc = EX_SYSERR;
> +		goto mount_exit;

This function later forks, so if you allocate before the fork, you need
to free in parent and in the child.
> @@ -2228,6 +2252,7 @@ mount_retry:
>  				if (nextaddress)
>  					*nextaddress++ = '\0';
>  			}
> +			memset(options, 0, sizeof(*options));
>  			goto mount_retry;

Altho not wrong this is a bit misleading as options is a char*. If you
do a memset do it option_size, or do options[0] = 0;

>  		case ENODEV:
>  			fprintf(stderr,
> @@ -2250,6 +2275,21 @@ mount_retry:
>  				already_uppercased = 1;
>  				goto mount_retry;
>  			}

Need to reset options again before goto I guess. Maybe reset option
after the retry label?

>  	}
> -	free(options);
> -	free(orgoptions);
> -	free(mountpoint);
> +	if (reinit_parsed_info)
> +		free(reinit_parsed_info);
> +	if (options)
> +		free(options);
> +	if (orgoptions)
> +		free(orgoptions);
> +	if (mountpoint)
> +		free(mountpoint);
>  	return rc;

free(NULL) is defined to be a no-op, you don't need the checks.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)