[PATCH] Fix bug 12572 - fd_open_atomic() can loop indefinitely when trying to open an invalid symlink with O_CREAT

[Ralph Böhme]

On Wed, Feb 15, 2017 at 04:15:48PM -0800, Jeremy Allison wrote:
> On Wed, Feb 15, 2017 at 10:14:38PM +0100, Ralph Böhme wrote:
> > Hi Jeremy,
> > 
> > On Wed, Feb 15, 2017 at 09:43:30AM -0800, Jeremy Allison wrote:
> > > Took a lot of thinking about but here is the
> > > patch I'm happy with to fix:
> > > 
> > > BUG: https://bugzilla.samba.org/show_bug.cgi?id=12572
> > > 
> > > plus a regression test to ensure it stays fixed :-).
> > > 
> > > Details can be found in the commit and code comments.
> > > 
> > > Please review and push if happy !
> > 
> > as we'll be limitting the loop to two iterations maybe we could break it up into
> > two simple subsequent calls? Attached is my first attempt at that, passes your
> > new test and a "make test TESTS=samba3.smbtorture_s3.plain". What do you think?
> > 
> > I'll need a few more passes over it, not 100% sure the logic is still the same,
> > this is just what I came up with in the first attempt.
> 
> OK, did some more thinking about the O_NOFOLLOW case and
> it's we're probably not going to be able to put the while(1)
> loop back anyway due to the few cases where we need to follow
> the symlink server-side. So how about this - based on your
> code (so dual ownership :-) that unrolls the loop but
> also (IMHO) makes the logic clearer w.r.t. the
> file_existed variable.
> 
> Feel free to push if happy !

happy with a minor indentation fix plus correct multiline if statement wrapping:

--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -9708,8 +9708,8 @@ static bool run_symlink_open_test(int dummy)
         * we use O_NOFOLLOW on the server or not.
         */
        if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND) ||
-                       NT_STATUS_EQUAL(status,
-                               NT_STATUS_OBJECT_PATH_NOT_FOUND)) {
+           NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_PATH_NOT_FOUND))
+       {
                correct = true;
        } else {
                printf("cli_ntcreate of %s returned %s - should return"

Otherwise lgtm, pushed. This is a really nice improvement of fd_open_atomic()
imho.

Cheerio!
-slow