 |
 |
@abh3 requested changes on this pull request.
Some suggested changes and some needed changes. I may not have caught all the issues here but it's an iterative process.
In src/XrdCrypto/XrdCryptosslAux.cc:
> @@ -485,9 +516,9 @@ int XrdCryptosslX509ParseFile(const char *fname,
rewind(fcer);
RSA *rsap = 0;
if (!PEM_read_RSAPrivateKey(fcer, &rsap, 0, 0)) {
- DEBUG("no RSA private key found in file "<<fname);
+ DEBUG("no RSA private key found in file");
Why is it that you are dumbing down the error message? Frankly, I would really want to know what file it is talking about. This error is now rather useless.
In src/XrdCrypto/XrdCryptosslAux.cc:
> } else {
- DEBUG("found a RSA private key in file "<<fname);
+ DEBUG("found a RSA private key in file");
Same here.
In src/XrdCrypto/XrdCryptosslAux.cc:
> @@ -538,9 +569,6 @@ int XrdCryptosslX509ParseFile(const char *fname,
}
}
- // We can close the file now
- fclose(fcer);
-
Where does this file get actually closed now?
In src/XrdTpc/XrdTpcNSSSupport.hh:
> +// Forward dec'ls.
+class XrdSysError;
+typedef void CURL;
+
+namespace TPC {
+
+/**
+ * libcurl with the NSS backend has significant memory leaks around the CA handling
+ * code. We have discovered that the memory leaks are *smallest* when NSS is given
+ * all the CA certificates in a single file (as opposed to many files in a directory).
+ *
+ * This class takes a traditional grid CA directory, parses its contents, and creates
+ * a single file.
+ *
+ * Each restart of the server this temporary file is created; further, every hour a
+ * new copy of the CAs is made.
This really needs to be configurable. CA's rarely change every hour and a site might just want to have a better fit for it schedule like every 12 hours.
In src/XrdTpc/XrdTpcNSSSupport.cc:
> +
+bool
+XrdTpcNSSSupport::NeedsNSSHack()
+{
+ const auto *version_info = curl_version_info(CURLVERSION_NOW);
+
+ if (!version_info->ssl_version) {return true;}
+ std::string ssl_version(version_info->ssl_version);
+
+ auto iter = ssl_version.find("NSS");
+ return iter != std::string::npos;
+}
+
+
+bool
+XrdTpcNSSSupport::Maintenance()
I know you are going to hate it but there is a large potential of leaking file descriptors all over this code. Please use the functions in XrdSysFD.hh which makes sure that file descriptors are not passed to forked processes.
In src/XrdTpc/XrdTpcNSSSupport.cc:
> + close(m_fd);
+ }
+}
+
+
+XrdTpcNSSSupport::TempCAGuard::TempCAGuard(int fd, const std::string &fname)
+ : m_fd(fd), m_fname(fname)
+ {}
+
+
+XrdTpcNSSSupport::XrdTpcNSSSupport(XrdSysError *err, std::string ca_dir)
+ : m_log(*err),
+ m_ca_dir(ca_dir),
+ m_plugin(err, &g_ver, "Crypto Support", "libXrdCryptossl.so")
+{
+ if (!(m_x509_to_file_func = m_plugin.Resolve("XrdCryptosslX509ToFile")) ||
Why in the world are you manually resolving these? The functions are available in standard shared libraries that you can link with. Presumably, this code is so critical to prevent HttpTpc from blowing up that you would really want it to work instead of just skipping all of this if you can't find the methods. Plus it makes no sense anyway since the libraries where you added these methods will be distributed with the code that uses them. So, why do this?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
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