Bug 13172 - smbclient symlink command reverses source and destination targets when creating reparse points on Windows.
smbclient symlink command reverses source and destination targets when creati...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 13159
  Show dependency treegraph
 
Reported: 2017-11-29 21:06 UTC by Jeremy Allison
Modified: 2017-12-14 11:05 UTC (History)
3 users (show)

See Also:


Attachments
git-am fix for master. (8.69 KB, patch)
2017-11-29 22:14 UTC, Jeremy Allison
no flags Details
git-am fix for 4.7.next. (7.60 KB, patch)
2017-11-30 23:06 UTC, Jeremy Allison
jra: review? (vl)
metze: review+
Details
git-am fix for 4.6.next (7.62 KB, patch)
2017-11-30 23:23 UTC, Jeremy Allison
jra: review? (vl)
metze: review+
Details
Text file containing the text for the 4.7.x, 4.6.x WHATSNEW. (521 bytes, patch)
2017-12-01 16:59 UTC, Jeremy Allison
metze: review-
Details
Updated WHATSNEW.txt change addressing metze's comments. (903 bytes, patch)
2017-12-07 16:51 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2017-11-29 21:06:25 UTC
Because we don't support reparse points in smbd, this code screwed up and reversed the newname -> oldname links when creating a reparse point against Windows.

Patch to follow.
Comment 1 Jeremy Allison 2017-11-29 22:14:22 UTC
Created attachment 13824 [details]
git-am fix for master.
Comment 2 Jeremy Allison 2017-11-30 23:06:25 UTC
Created attachment 13832 [details]
git-am fix for 4.7.next.

Cherry-pick from master (minus WHATSNEW.txt change).
Comment 3 Jeremy Allison 2017-11-30 23:23:37 UTC
Created attachment 13833 [details]
git-am fix for 4.6.next

Back-ported from master - minus WHATSNEW.txt fix.
Comment 4 Jeremy Allison 2017-11-30 23:26:03 UTC
How should I handle the WHATNEW.txt change ? It looks like it's being written by Karolin when a release is created, not from the normal patch process ?

This is what went into master:

-----------------------------------------------------
smbclient reparse point symlink parameters reversed
===================================================

A bug in smbclient caused the 'symlink' command to reverse the
meaning of the new name and link target parameters when creating a
reparse point symlink against a Windows server. As this is a
little used feature the ordering of these parameters has been
reversed to match the parameter ordering of the UNIX extensions
'symlink' command. The usage message for this command has also
been improved to remove confusion.
-----------------------------------------------------

Should we just ask her to include this when doing the release ?

Jeremy.
Comment 5 Stefan Metzmacher 2017-12-01 06:55:27 UTC
(In reply to Jeremy Allison from comment #4)

Typically WHATSNEW stuff for master, gets in as normal.

At the same time I'm sure Karolin will also be happy to just
get a small text section instead of a git am patch and create the patch herself.
Comment 6 Jeremy Allison 2017-12-01 16:59:10 UTC
Created attachment 13835 [details]
Text file containing the text for the 4.7.x, 4.6.x WHATSNEW.

So I'm adding a .txt file including the change I'd like to see in the WHATSNEW.txt for Karolin to review. I can't see where I'd add this to the current 4.7.next and 4.6.next WHATSNEW.txt files as each new release seems to generate a stanza containing all the bug fixes, listed by creating engineer. Hope this is OK.

Cheers,

Jeremy.
Comment 7 Volker Lendecke 2017-12-04 14:56:13 UTC
Not sure about this one. I see that we can change behaviour between releases, but within a release? I concur that this is probably not the most-used feature, but do you really want to break user scripts within a release?
Comment 8 Jeremy Allison 2017-12-04 16:41:07 UTC
Hmmm. Good point. I guess it just bugs me that symlink a -> b means symlink b -> a when done against Windows right now.

I'm happy with it in master, so I can go either way on the release branches.

Re-assigning to Karolin so we can get her to chip in with an "official" opinion.

Karolin, what do you think ?

Jeremy.
Comment 9 Karolin Seeger 2017-12-05 08:53:05 UTC
(In reply to Jeremy Allison from comment #8)
Currently, I tend to change it with the next major release, but I will think about it a bit more.
Comment 10 Stefan Metzmacher 2017-12-06 09:50:48 UTC
(In reply to Jeremy Allison from comment #4)

It's not clear to me if the behavior has actually changed when using
SMB1 unix extensions?

Did only the code path without unix extensions has the reverse problem?
Comment 11 Jeremy Allison 2017-12-06 17:07:26 UTC
Yes, that is correct - the SMB1 UNIX code path is unchanged. The error occurred when making the symlink command create reparse points on a Windows server via the SMB1 trans2 request. The newname and target arguments were reversed. We didn't notice as smbd doesn't support reparse points so didn't test it. This patch fixes that so that symlink <newpath> <target> means the same for both Windows reparse points and UNIX extension symlinks. I discovered it when creating SMB2 code to create Windows reparse points in libsmb.

So it's an external visible parameter change, but not in anything that was widely used (or known about probably).

It's part of my plan to create a full test suite for reparse points before implementing in smbd (we need this to implement SMB2 UNIX extensions).
Comment 12 Stefan Metzmacher 2017-12-07 06:29:49 UTC
Comment on attachment 13835 [details]
Text file containing the text for the 4.7.x, 4.6.x WHATSNEW.

Ok, then the WHATSNEW entry need to make this clearer, that there's
no change for the common case. And the special case against windows
now work in the same way as the common case.

The current one indicates to me that we just changed the command line
interface and reverse the arguments and all users need to change their
scripts.
Comment 13 Karolin Seeger 2017-12-07 09:53:17 UTC
Ok, as the same command behaves different against varying servers, it makes sense to correct the behavior with bugfix releases. Objections?
Comment 14 Karolin Seeger 2017-12-07 09:57:17 UTC
Today is freeze for Samba 4.7.4. If someone would add the review flag today, I could pick the fix...
Comment 15 Jeremy Allison 2017-12-07 16:51:43 UTC
Created attachment 13851 [details]
Updated WHATSNEW.txt change addressing metze's comments.
Comment 16 Stefan Metzmacher 2017-12-07 19:02:22 UTC
Comment on attachment 13851 [details]
Updated WHATSNEW.txt change addressing metze's comments.

Thanks, Jeremy! Looks much better:-)
Comment 17 Jeremy Allison 2017-12-08 16:25:01 UTC
Re-assigned to Karolin for inclusion in 4.7.next, 4.6.next.
Comment 18 Karolin Seeger 2017-12-14 11:05:35 UTC
Pushed to both branches.
Closing out bug report.

Thanks!