[PATCH] Remove lp_posix_pathname() from synthetic_smb_fname_split()

[Uri Simchoni]

A couple of comments:

In 2/6:
+	if (lp_posix_pathnames()) {
+		/* No stream name looked for. */
+		return synthetic_smb_fname(ctx, fname, NULL, NULL);

Looks to me like we need psbuf here in the last parameter (sure, it's 
removed one patch later but still... unless I'm missing something)

In 6/6 last hunk:
  			if (tmp == NULL) {
  				status = NT_STATUS_NO_MEMORY;
  				TALLOC_FREE(fname_dst_parent);
-				TALLOC_FREE(smb_fname_orig_lcomp);
+				TALLOC_FREE(orig_lcomp_path);
+				TALLOC_FREE(orig_lcomp_stream);
  				goto out;
  			}
  			TALLOC_FREE(smb_fname_dst->stream_name);
-			smb_fname_dst->stream_name = tmp;
+			smb_fname_dst->stream_name = orig_lcomp_stream;
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Do we want that last replacement? looks like use-after-free. We can use 
talloc_steal() if we want to be clever, or just leave it as-is - assign 
recently-allocated tmp.


Otherwise RB+ me.

Thanks,
Uri.

On 03/10/2016 07:09 AM, Jeremy Allison wrote:
> OK, 6 patch set here that removes
> one of the core pain-points of
> lp_posix_pathnames() - its use
> within synthetic_smb_fname_split().
>
> It also refactors some horrible
> code (I wrote many years ago) in
> rename_internals_fsp() that
> abused the struct smb_filename
> struct for convenience, when
> all I really should have used
> was 2 char * pointers...
>
> Patches in order:
>
> 1). Adds split_stream_filename()
> function. Will be used in 2 places
> in patches to follow.
>
> 2). Rewrites internals of synthetic_smb_fname_split()
> to use the recently added split_stream_filename().
> Code is much clearer.
>
> 3). Remove const SMB_STRUCT_STAT * parameter
> from synthetic_smb_fname_split(). Only one
> caller uses this, and is much clearer to
> handle externally.
>
> 4). Remove lp_posix_pathnames() from
> synthetic_smb_fname_split(). Pass as
> a parameter instead.
>
> This adds lp_posix_pathnames() rather
> than removing them, but mostly the places
> added are 'leaf' code - right outside
> of the smbd code (mostly in the test
> funtions used by vfstest where this
> can eventually be replaced with an
> environment variable lookup if we
> ever need it). The one place inside
> smbd where we add this will be removed
> in patch #6.
>
> 5). Simplify logic inside rename_internals_fsp(),
> part 1. Use standard parent_dirname() instead
> of hand-crafted code that does the same thing.
>
> 6). Simplify logic inside rename_internals_fsp(),
> part 2. Uses split_stream_filename() instead
> of synthetic_smb_fname_split() to clean up
> the logic and make is clearer. Removes
> lp_posix_pathnames() added in patch #4.
>
> Passes local make test.
>
> Please review and push if happy.
>
> We're well on the way to having
> posix paths be a request-specific
> variable inside smbd !
>
> Cheers,
>
> 	Jeremy.
>