Print

---

@abh3 requested changes on this pull request.

There are some serious issues here. Most are about long term maintenance and the requested code changes should address those. That aside, the rename implementation is problematic.

In src/XrdOfs/XrdOfs.cc:

> @@ -2229,9 +2246,18 @@ int XrdOfs::rename(const char             *old_name,  // In
 
 // Apply security, as needed
 //
-   AUTHORIZE2(client, einfo,
-              AOP_Rename, "renaming",    old_name, &old_Env,
-              AOP_Insert, "renaming to", new_name, &new_Env );
+   // Make a copy of the XrdSecEntity object xattrs as the authorization below may change it.

I see the comment but there is no code that corresponds to this comment.

In src/XrdOfs/XrdOfs.cc:

> @@ -2229,9 +2246,18 @@ int XrdOfs::rename(const char             *old_name,  // In
 
 // Apply security, as needed
 //
-   AUTHORIZE2(client, einfo,
-              AOP_Rename, "renaming",    old_name, &old_Env,
-              AOP_Insert, "renaming to", new_name, &new_Env );
+   // Make a copy of the XrdSecEntity object xattrs as the authorization below may change it.
+   AUTHORIZE(client, &old_Env, AOP_Rename, "renaming", old_name, einfo);
+   if (client) client->eaAPI->Add("request.name", "", true);

OK, I see lots of code changes that depend on whether this is empty or not and this is where it is made empty. We really need a comment on why this is occurring and why it is being set here to null. Once I know why this is being done I mat be able to suggest an alternative that doesn't affect every current and possible future use of the key.

In src/XrdOfs/XrdOfs.cc:

> @@ -2252,10 +2278,29 @@ int XrdOfs::rename(const char             *old_name,  // In
        evsObject->Notify(XrdOfsEvs::Mv, evInfo);
       }
 
+// If we cannot overwrite, we must open-exclusive first.  This will test whether
+// we will destroy data in the rename (without actually destroying data).
+//
+   std::unique_ptr<XrdSfsFile> tmp_fp = nullptr;
+   if (cannot_overwrite)
+      {tmp_fp.reset(newFile(einfo));
+       if (!tmp_fp)
+          {return fsError(einfo, ENOMEM);}
+       if (SFS_OK != tmp_fp->open(new_name, SFS_O_CREAT, 0700, client, infoN))

This really is not going to work for two reasons. First, using the SFS to do this via open is asking for trouble since the open and rename privileges are totally independent. In other words, the client may be able to clobber a file via open but not clobber it via rename. Or the token may only allow renames but not opens. Using this approach may lead to inconsistent behavior. Secondly, If the server crashes right after a successful open then you are left with a dangling file and subsequent logically valid operations on that file may fail (e.g. the same client retries the rename). Plus, I think this can be simplified; especially once one realizes there is no way to avoid race conditions between open and rename so any side-effects better be idempotent. Frankly, I don't see why you simply can't ask the oss whether the target file exists or not and if it exists don't allow the rename. No side-effects usually works just right. OK, not 100% right because of possible race conditions but the present solution has the same problem plus it introduces an unwanted side-effect.

In src/XrdOfs/XrdOfs.cc:

>  // Perform actual rename operation
 //
    if ((retc = XrdOfsOss->Rename(old_name, new_name, &old_Env, &new_Env)))
-      return XrdOfsFS->Emsg(epname, einfo, retc, "rename", old_name);
+      {if (tmp_fp)
+          {tmp_fp.reset();
+           // Try cleaning up the destination file we temporarily created.
+           if (SFS_OK != remove('f', new_name, einfo, client, infoN))

We really like to avoid these kinds of problems so it's best not to introduce the possibility in the first place.

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -87,6 +124,48 @@ XrdAccPrivs AddPriv(Access_Operation op, XrdAccPrivs privs)
     return static_cast<XrdAccPrivs>(new_privs);
 }
 
+const std::string OpToName(Access_Operation op) {

Same thing here as there are now three implementations of this.

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -252,6 +331,26 @@ class XrdAccRules
         return "";
     }
 
+    const std::string str() const

Hmmm, I recall this was in the previous commit I just merged. So, I suspect this will cause a conflict.

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> +        if ((result = scitokens_conf.Gather(config_filename, XrdOucGatherConf::trim_lines)) < 0) {
+            m_log.Emsg("Config", -result, "parsing config file", config_filename);
+            return false;
+        }
+
+        char *val;
+        std::string map_filename;
+        while (scitokens_conf.GetLine()) {
+            m_log.setMsgMask(0);
+            scitokens_conf.GetToken(); // Ignore the output; we asked for a single config value, trace
+            if (!(val = scitokens_conf.GetToken())) {
+                m_log.Emsg("Config", "scitokens.trace requires an argument.  Usage: scitokens.trace [all|error|warning|info|debug|none]");
+                return false;
+            }
+            do {
+                if (!strcmp(val, "all")) {m_log.setMsgMask(m_log.getMsgMask() | LogMask::All);}

This can also be part of the abstraction since its the same code in six places.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <xrootd/xrootd/pull/1697/review/961337609@github.com>

Use REPLY-ALL to reply to list

To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1