[Python-checkins] gh-95041: Fix several minor issues in syslog.openlog() (GH-95058)

[serhiy-storchaka]

https://github.com/python/cpython/commit/68c555a50a2b74731b0db0f4dcbf51b2c11d4853
commit: 68c555a50a2b74731b0db0f4dcbf51b2c11d4853
branch: main
author: Serhiy Storchaka <storchaka at gmail.com>
committer: serhiy-storchaka <storchaka at gmail.com>
date: 2022-07-26T09:12:10+03:00
summary:

gh-95041: Fix several minor issues in syslog.openlog() (GH-95058)

* syslog_get_argv() swallows exceptions, but not in all cases.
* if ident is non UTF-8 encodable, syslog.openlog() fails after setting the
  global reference to ident. Now the C string saved internally in the previous
  call to openlog() points to the freed memory.
* PySys_Audit() can crash if ident is NULL.
* There may be a race condition with syslog.syslog(), because the global
  reference to ident is decrefed before setting the new value.
* Possible use of freed memory if syslog.openlog() is called while
  the GIL is released in syslog.syslog().

files:
M Modules/syslogmodule.c

diff --git a/Modules/syslogmodule.c b/Modules/syslogmodule.c
index 2655a0c94bbd9..27b176aeb6dba 100644
--- a/Modules/syslogmodule.c
+++ b/Modules/syslogmodule.c
@@ -87,6 +87,10 @@ syslog_get_argv(void)
     }
 
     scriptobj = PyList_GetItem(argv, 0);
+    if (scriptobj == NULL) {
+        PyErr_Clear();
+        return NULL;
+    }
     if (!PyUnicode_Check(scriptobj)) {
         return(NULL);
     }
@@ -96,16 +100,16 @@ syslog_get_argv(void)
     }
 
     slash = PyUnicode_FindChar(scriptobj, SEP, 0, scriptlen, -1);
-    if (slash == -2)
+    if (slash == -2) {
+        PyErr_Clear();
         return NULL;
+    }
     if (slash != -1) {
         return PyUnicode_Substring(scriptobj, slash + 1, scriptlen);
     } else {
         Py_INCREF(scriptobj);
         return(scriptobj);
     }
-
-    return(NULL);
 }
 
 
@@ -114,42 +118,41 @@ syslog_openlog(PyObject * self, PyObject * args, PyObject *kwds)
 {
     long logopt = 0;
     long facility = LOG_USER;
-    PyObject *new_S_ident_o = NULL;
+    PyObject *ident = NULL;
     static char *keywords[] = {"ident", "logoption", "facility", 0};
-    const char *ident = NULL;
+    const char *ident_str = NULL;
 
     if (!PyArg_ParseTupleAndKeywords(args, kwds,
-                          "|Ull:openlog", keywords, &new_S_ident_o, &logopt, &facility))
+                          "|Ull:openlog", keywords, &ident, &logopt, &facility))
         return NULL;
 
-    if (new_S_ident_o) {
-        Py_INCREF(new_S_ident_o);
+    if (ident) {
+        Py_INCREF(ident);
     }
-
-    /*  get sys.argv[0] or NULL if we can't for some reason  */
-    if (!new_S_ident_o) {
-        new_S_ident_o = syslog_get_argv();
+    else {
+        /* get sys.argv[0] or NULL if we can't for some reason  */
+        ident = syslog_get_argv();
     }
 
-    Py_XDECREF(S_ident_o);
-    S_ident_o = new_S_ident_o;
-
-    /* At this point, S_ident_o should be INCREF()ed.  openlog(3) does not
-     * make a copy, and syslog(3) later uses it.  We can't garbagecollect it
+    /* At this point, ident should be INCREF()ed.  openlog(3) does not
+     * make a copy, and syslog(3) later uses it.  We can't garbagecollect it.
      * If NULL, just let openlog figure it out (probably using C argv[0]).
      */
-    if (S_ident_o) {
-        ident = PyUnicode_AsUTF8(S_ident_o);
-        if (ident == NULL)
+    if (ident) {
+        ident_str = PyUnicode_AsUTF8(ident);
+        if (ident_str == NULL) {
+            Py_DECREF(ident);
             return NULL;
+        }
     }
-
-    if (PySys_Audit("syslog.openlog", "sll", ident, logopt, facility) < 0) {
+    if (PySys_Audit("syslog.openlog", "Oll", ident ? ident : Py_None, logopt, facility) < 0) {
+        Py_DECREF(ident);
         return NULL;
     }
 
-    openlog(ident, logopt, facility);
+    openlog(ident_str, logopt, facility);
     S_log_open = 1;
+    Py_XSETREF(S_ident_o, ident);
 
     Py_RETURN_NONE;
 }
@@ -193,9 +196,15 @@ syslog_syslog(PyObject * self, PyObject * args)
         }
     }
 
+    /* Incref ident, because it can be decrefed if syslog.openlog() is
+     * called when the GIL is released.
+     */
+    PyObject *ident = S_ident_o;
+    Py_XINCREF(ident);
     Py_BEGIN_ALLOW_THREADS;
     syslog(priority, "%s", message);
     Py_END_ALLOW_THREADS;
+    Py_XDECREF(ident);
     Py_RETURN_NONE;
 }
 
@@ -355,4 +364,4 @@ PyMODINIT_FUNC
 PyInit_syslog(void)
 {
     return PyModuleDef_Init(&syslogmodule);
-}
\ No newline at end of file
+}