@davidlt commented on this pull request.

In src/XrdApps/XrdMpxStats.cc:

> @@ -473,6 +473,7 @@ int main(int argc, char *argv[])
                  else {Say.Emsg(":", "Invalid format - ", optarg); Usage(1);}
                  break;
        case 'h': Usage(0);
+                 break;

Usage(0) will call exit(0), but adding break is an extra failsafe that shouldn't hurt.

In src/XrdCl/XrdClZipArchiveReader.cc:

> @@ -243,7 +243,7 @@ class ZipArchiveReaderImpl
     {
       delete pBuffer;
       ClearRecords();
-      pArchive.Close();
+      XRootDStatus st = pArchive.Close();

The declaration of Close() has an attribute to check return value, but I could't figure out what to do (we are in destructor). Just saving return value tricks the compiler to avoid the warning, but probably is not the right approach.

In src/XrdClient/XrdClient.cc:

> @@ -1372,15 +1372,15 @@ bool XrdClient::OpenFileWhenRedirected(char *newfhandle, bool &wasopen)
 	Info(XrdClientDebug::kHIDEBUG,
 	     "OpenFileWhenRedirected", "Stripping off the 'delete' option." );
 
-	options &= !kXR_delete;

This does not seem right at the first look. kXR_delete is a positive integer (enum), which would be translated to true here and negating that would result in false (i.e. 0).

In src/XrdClient/XrdClient.cc:

>  	options |= kXR_open_updt;
     }
 
     if (fOpenPars.options & kXR_new) {
 	Info(XrdClientDebug::kHIDEBUG,
 	     "OpenFileWhenRedirected", "Stripping off the 'new' option." );
 
-	options &= !kXR_new;

Same here.

In src/XrdCms/XrdCmsManTree.cc:

> @@ -79,7 +79,7 @@ int XrdCmsManTree::Connect(int nID, XrdCmsNode *nP)
 {
    static CmsDiscRequest discRequest = {{0, kYR_disc, 0, 0}};
    XrdSysMutexHelper Monitor(myMutex);
-   char mybuff[8];
+   char mybuff[16];

Compiler now studies string format to understand what size buffer is required (min and max bytes). According to compiler this buffer (an many other) are too small to fit potential values. Note that in most (or all) cases snprintf is used thus we don't have buffer overflow, but string can be truncated.

In src/XrdCms/XrdCmsReq.cc:

> @@ -134,17 +134,17 @@ void XrdCmsReq::Reply_Error(int ecode, const char *emsg, int elen)
 // Translate the error name
 //
    switch(ecode)
-         {case ENOENT:       eval = kYR_ENOENT;
-          case EPERM:        eval = kYR_EPERM;
-          case EACCES:       eval = kYR_EACCES;
-          case EIO:          eval = kYR_EIO;
-          case ENOMEM:       eval = kYR_ENOMEM;
-          case ENOSPC:       eval = kYR_ENOSPC;
-          case ENAMETOOLONG: eval = kYR_ENAMETOOLONG;
-          case ENETUNREACH:  eval = kYR_ENETUNREACH;
-          case ENOTBLK:      eval = kYR_ENOTBLK;
-          case EISDIR:       eval = kYR_EISDIR;
-          default:           eval = kYR_EINVAL;

In this case whatever ecode is we set eval to kYR_EINVAL. This looks wrong, thus probably we forgot to add breaks otherwise the whole thing can be reduced to a single line: eval = kYR_EINVAL;.

In src/XrdCns/XrdCnsConfig.cc:

> @@ -150,6 +150,7 @@ int XrdCnsConfig::Configure(int argc, char **argv, char *argt)
                     else NoGo = NAPath("'-a'", Spec.argval);
                  break;
        case 'B': Opts |= optNoCns;
+                 [[gnu::fallthrough]];

Fallthroughs must be explicitly marked. Note, that [[fallthrough]] attribute was added to C++17 and GNU makes it available in C++11 and C++14 via [[gnu::fallthrough]]. Note that Clang had for years [[gnu::fallthrough]]. One can also use GNU style attribute. They also support simple C++ comments with specific patterns. Read more here: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html (Search for -Wimplicit-fallthrough).

In src/XrdCns/XrdCnsLogRec.hh:

> @@ -58,8 +58,8 @@ struct Ctl
 
 struct Arg
       {char Type;                   // Event code
-       char Mode[3];                // Mode (create, inv, mkdir)
-       char SorT[12];               // Size (closew, inv) | TOD (eol)

In these two cases we always lost the last char, because snprintf needs to write terminating NULL byte. Thus we need to increase buffers in order not to loose the last digit.

In src/XrdCrypto/XrdCryptosslX509.cc:

> @@ -507,8 +507,8 @@ const char *XrdCryptosslX509::IssuerHash(int alg)
       if (issueroldhash.length() <= 0) {
          // Make sure we have a certificate
          if (cert) {
-            char chash[15] = {0};
-            snprintf(chash,15,"%08lx.0",X509_NAME_hash_old(cert->cert_info->issuer));
+            char chash[30] = {0};
+            snprintf(chash, sizeof(chash), "%08lx.0", X509_NAME_hash_old(cert->cert_info->issuer));

I don't fully understand this string format. Why do you need width set to 8 and also ".0" at the end. Just looks a bit awkward. Does your hash really end with ".0"?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

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