Description of problem: While running debug builds the pem module sometimes crashes while attempting to write to its log file. The problem occurs when the module is used initially by an application being run as root and later by the same or another application running as a regular user. Version-Release number of selected component (if applicable): nss-3.12.9-14. How reproducible: often Steps to Reproduce: 1. Use an application such as crypto-util's genkey untility to generate a self-signed certificate for an apache server or a CA. Such an operation must be run as root. 2. Run a application, e.g. curl, that will gets its cryptographic services by the pem module. Actual results: The pem module crashes on initialization. Expected results: The application should complete it's task normally Additional info:
The PEM module's initialization function pem_Initialize we has .... open_log(); plog("pem_Initialize\n"); .... Looing is implemented in util.c with FILE *plogfile; void open_log() { #ifdef DEBUG plogfile = fopen("/tmp/pkcs11.log", "a"); #endif return; } The fopen tries to open the log file to write to it. If the log file was initilially created by an application running as root fopen will fail and plogfile will be set to NULL. Later plog calls vfprintf(plogfile, fmt, ap); with a NULL and a crash happens. Using the logging utilities from NSPR will prevent this problems. It is also what's required of modules intended to be part of NSS as this one is.
Created attachment 490990 [details] Reimplement PEM logging in terms of NSPR's own
Comment on attachment 490990 [details] Reimplement PEM logging in terms of NSPR's own >@@ -267,34 +271,35 @@ ReadDERFromFile(SECItem *** derlist, char *filename, PRBool ascii, > return -1; > } > >-FILE *plogfile; >+#define PEM_DEFAULT_LOG_FILE "/tmp/pkcs11.log" >+ >+static const char *pemLogModuleName = "PEM"; >+ >+PRLogModuleInfo* pemLogModule; pemLogModule could be static. Both definitions should also be wrapped by "#ifdef DEBUG" so that they do not provoke picky compilers/analyzers when building without DEBUG. > void open_log() > { > #ifdef DEBUG >- plogfile = fopen("/tmp/pkcs11.log", "a"); >-#endif >+ const char *nsprLogFile = PR_GetEnv("NSPR_LOG_FILE"); > >- return; >-} >+ pemLogModule = PR_NewLogModule(pemLogModuleName); The above should be indented by 4 spaces, not a tab. >-void close_log() >-{ >-#ifdef DEBUG >- fclose(plogfile); >+ (void) PR_SetLogFile(nsprLogFile ? nsprLogFile : PEM_DEFAULT_LOG_FILE); >+ /* If false, the log file will remain what it was before */ > #endif >- return; > } The above should be indented by 4 spaces, not a tab. >+#define LOGGING_BUFFER_SIZE 400 >+ > void plog(const char *fmt, ...) > { > #ifdef DEBUG > va_list ap; >+ char buf[LOGGING_BUFFER_SIZE]; > > va_start(ap, fmt); >- vfprintf(plogfile, fmt, ap); >+ PR_vsnprintf(buf, sizeof(buf), fmt, ap); > va_end(ap); >- >- fflush(plogfile); >+ PR_LOG(pemLogModule, PR_LOG_DEBUG, ("%s", buf)); > #endif > } This looks like a reasonable mid-term solution. The more ambitious solution would be to replace all the calls of plog() by PR_LOG() in order to avoid computation of the output when not printed anyway. This would probably allow us to drop the #ifdefs completely and compile the debugging support by default. But for now, I am fine with this solution.
(In reply to comment #3) > pemLogModule could be static. Both definitions should also be wrapped by > "#ifdef DEBUG" so that they do not provoke picky compilers/analyzers when > building without DEBUG. Yes > The above should be indented by 4 spaces, not a tab. > The above should be indented by 4 spaces, not a tab. Sure, I'll fix it. > This looks like a reasonable mid-term solution. The more ambitious solution > would be to replace all the calls of plog() by PR_LOG() in order to avoid > computation of the output when not printed anyway. This would probably allow > us to drop the #ifdefs completely and compile the debugging support by default. > But for now, I am fine with this solution. Yes, this short-term (and not to disruptive solution) is prudent given that f-15 is going beta. The more ambitious solution is in progress and is best saved for the NSS upstream submission.
Created attachment 491545 [details] Patch V2: with changes from Kamil's review
nss-3.12.9-15.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/nss-3.12.9-15.fc15
nss-3.12.9-10.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/nss-3.12.9-10.fc14
Package nss-3.12.9-15.fc15: * should fix your issue, * was pushed to the Fedora 15 testing repository, * should be available at your local mirror within two days. Update it with: # su -c 'yum update --enablerepo=updates-testing nss-3.12.9-15.fc15' as soon as you are able to. Please go to the following url: https://admin.fedoraproject.org/updates/nss-3.12.9-15.fc15 then log in and leave karma (feedback).
nss-3.12.9-15.fc15 has been pushed to the Fedora 15 stable repository. If problems still persist, please make note of it in this bug report.
nss-3.12.9-10.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.