fix for possible bug in shadow_copy2 (4.7.5) plus minor enhancement

Fri Mar 16 16:10:01 UTC 2018

On Fri, Mar 09, 2018 at 11:14:01AM +0100, Thomas Nau via samba-technical wrote:
> Dear all
> 
> I found an issue when running 4.7.5 on a Solaris 11.3 server.
> It relates to the use of regex patterns in the shadow-copy2
> VFS module. When having set "shadow: localtime = yes"
> the Window7 client from time to time complains about a vanished
> or not existing snapshot while others work fine. Snapshots
> are created hourly and daily by the Solaris time-slider and
> named like
> 	pool/data at zfs-auto-snap_secretly-hourly-2018-02-07-15h14
> 	pool/data at zfs-auto-snap_secretly-daily-2018-03-07-16h14
> 
> 
> Debugging lead to the following and I hope you can confirm.
> When using this feature the code path is slightly different
> when a client selects a snapshot from the list. Samba has to
> lookup the pattern in an internal list which seems to be setup
> being based on GMT time information.
> 
> The ZFS snapshots created by time-slider in Solaris, and probably
> by most other tools too, are using localtime for the filename
> being created. Samba does the required conversion to GMT when
> setting up the list and before sending it to the client. The
> information being sent back by the client is GMT too.
> Given that we should not convert to localtime before searching
> through the list.
> If you are not on GMT but use hourly snapshots windows might
> actually present you a folder or file but it will not be for
> the timestamp you expect but simply match another hourly one.

I'll try and take a look at this in the next week or so.

Thanks a *LOT* for looking at this !

Can you send in the Samba DCO form (can be found here):

https://www.samba.org/samba/devel/copyright-policy.html

so I know we've got the rights to merge your patch
if it passes review.

Cheers,

	Jeremy.

> The other thing is about an improved handling of snapshot names.
> Given Solaris time-slider, and probably other tools, creates names
> such as
> 	pool/data at zfs-auto-snap_smb-hourly-2018-02-07-15h14
> 	pool/data at zfs-auto-snap_smb-daily-2018-03-07-16h14
> we would need the regex-fromat feature plus a rather unpleasant
> pattern for the delimiter and the time format like
> 	shadow: snapprefix          = zfs-auto-snap_smb-
> 	shadow: delimiter           = ly-
> 	shadow: format              = ly-%Y-%m-%d-%Hh%M
> 
> So while this works as the time specifications all end by "ly"
> it becomes a pain if the delimiter pattern shows further to
> the left in the string.
> For example in
> 	pool/data at zfs-auto-snap_secretly-hourly-2018-02-07-15h14
> 	pool/data at zfs-auto-snap_secretly-daily-2018-03-07-16h14
> delimiter would split the name at "secret ly-" and the time format,
> not regex capable, cannot distinguish between daily and hourly anymore.
> 
> That said my approach keeps the old style valid but extends it a bit.
> In case you set the delimiter explicitly to be empty string
> 	shadow: delimiter =
> The patched code ignores the delimiter, matches the regex and assumes
> that the time format string starts right after the match. The patch
> applied allows for this config
>     shadow: snapprefix          = zfs-auto-snap_smb-[a-z]*-
>     shadow: delimiter           =
>     shadow: format              = %Y-%m-%d-%Hh%M
> 
> 
> I hope my findings are right and you find the attached diff useful
> 
> Thomas
> 
> 
> 

> --- samba-4.7.5.orig/source3/modules/vfs_shadow_copy2.c	2017-07-04 12:05:25.000000000 +0200
> +++ samba-4.7.5/source3/modules/vfs_shadow_copy2.c	2018-03-09 10:05:17.524283183 +0100
> @@ -279,9 +279,14 @@
>  			return -1;
>  		}
>  	} else {
> -		if (config->use_localtime) {
> +		/*
> +		 * In case the rexec option is used we will have to search
> +		 * the list of snapshots. As it's stored in GMT we cannot
> +		 * convert to localtime as in the other case.
> +		 */
> +		if (config->use_localtime && priv->snaps->regex == NULL) {
>  			if (localtime_r(&snapshot, &snap_tm) == 0) {
> -				DEBUG(10, ("gmtime_r failed\n"));
> +				DEBUG(10, ("localtime_r failed\n"));
>  				return -1;
>  			}
>  		} else {
> @@ -1918,6 +1923,7 @@
>  	char *tmp = NULL;
>  	bool converted = false;
>  	int ret = -1;
> +	regmatch_t pmatch[1];
>  
>  	SMB_VFS_HANDLE_GET_DATA(handle, priv, struct shadow_copy2_private,
>  				return NULL);
> @@ -1930,27 +1936,46 @@
>  	 * If regex is provided, then we will have to parse the
>  	 * filename which will contain both the prefix and the time format.
>  	 * e.g. <prefix><delimiter><time_format>
> +	 *
> +	 * As an extension of that we match and trim the regex <prefix
> +	 * when delimiter is set to an empty string. This allows for
> +	 * more flexible snapshot naming and more readable time
> +	 * format strings.
>  	 */
>  	if (priv->snaps->regex != NULL) {
> -		tmpstr = talloc_strdup(talloc_tos(), name);
> -		/* point "name" to the time format */
> -		name = strstr(name, priv->config->delimiter);
> -		if (name == NULL) {
> -			goto done;
> -		}
> -		/* Extract the prefix */
> -		tmp = strstr(tmpstr, priv->config->delimiter);
> -		if (tmp == NULL) {
> -			goto done;
> -		}
> -		*tmp = '\0';
> +		if (priv->config->delimiter != NULL && *(priv->config->delimiter)) {
> +			tmpstr = talloc_strdup(talloc_tos(), name);
> +			/* point "name" to the time format */
> +			name = strstr(name, priv->config->delimiter);
> +			if (name == NULL) {
> +				goto done;
> +			}
> +			/* Extract the prefix */
> +			tmp = strstr(tmpstr, priv->config->delimiter);
> +			if (tmp == NULL) {
> +				goto done;
> +			}
> +			*tmp = '\0';
>  
> -		/* Parse regex */
> -		ret = regexec(priv->snaps->regex, tmpstr, 0, NULL, 0);
> -		if (ret) {
> -			DBG_DEBUG("shadow_copy2_snapshot_to_gmt: "
> -				  "no regex match for %s\n", tmpstr);
> -			goto done;
> +			/* Parse regex */
> +			ret = regexec(priv->snaps->regex, tmpstr, 0, NULL, 0);
> +			if (ret) {
> +				DBG_DEBUG("shadow_copy2_snapshot_to_gmt: "
> +					  "no regex match for %s\n", tmpstr);
> +				goto done;
> +			}
> +		} else {
> +			/* Parse regex */
> +			ret = regexec(priv->snaps->regex, name, 1, pmatch, 0);
> +			if (ret) {
> +				DBG_DEBUG("shadow_copy2_snapshot_to_gmt: "
> +					  "no regex match for %s\n", tmpstr);
> +				goto done;
> +			}
> +			/* point "name" to time format right after the regex match */
> +			if (pmatch[0].rm_eo > 0) {
> +				name += pmatch[0].rm_eo;
> +			}
>  		}
>  	}
>  