This project is archived and is in readonly mode.
_mogrify leaks reference when same dict value referenced multiple times with $()
Discovered in 2.0.13, verified against 2.4.3 from git.
Note, title is wrong, I typed $() instead of %() but I can't find a way to fix it....
If _mogrify is called with an operation string and a dictionary and the operation string references a value from the dict using the %() syntax more than once, a reference to that dict value will be leaked for each reference after the first.
I attached a (very simple) patch for this wich fixes the problem, however being unfamiliar with the code it may not be correct from a wholistic perspective :) Please advise.
We at Red Hat are carrying psycopg2 2.0.13 and 2.0.14 in RHEL 5 and RHEL 6, and it is being packaged at Fedora as well (2.4.2 in F15, in F15+ also I believe). If someone can verify that the patch is legit and approve it, that would be great -- we can carry our own patch until it's available upstream.
I can also attach a simple test program which demonstrates the problem (I assume the bug system will let me submit an additional attachment after creation).
-
mogrify_reference_leak.patch
782 Bytes
-
Trevor McKayNote, the test program assumes a local database with the dsn ""dbname=cumin user=cumin host=localhost" just to get a connection so a cursor can be created. Modify as necessary, the call to mogrify is the important part.
-
psycopg_leak.py
905 Bytes
-
-
Trevor McKay
Note,
I found a patch for this already in version 2.3.2, but it appears not to be in the 2.4 tree (fyi).
-
Daniele Varrazzo- State changed from new to resolved
Good catch, thank you very much. Fixed in my devel.
What do you mean as "found a patch in 2.3.2"?
-
Trevor McKay
What do you mean as "found a patch in 2.3.2"
Sorry for the confusion on this. It appears that at least some Fedora and/or Red Hat distributions of psycopg2 have applied the fix for this in the past, but maybe the fix wasn't pushed back to upstream (or maybe it was lost, another possibility?). I didn't realize this until after I posted the comment :)
Still trying to sort out the history on our end, thank you for the fix.
T.
-
Trevor McKay
Okay,
Just to be clear for posterity. I was mistaken about any patch in 2.3.2, the error is there is well.
The code in 2.3.2 is using "item = PyObject_GetItem(n, key)" to check for presence of the value in the temporary dictionary, and so it rightly contains "Py_DECREF(item)" in the case where the value is found to already be in the dictionary. What it is missing is another call, Py_DECREF(value).
In newer versions, "item = PyObject_GetItem(n, key)" is replaced with "0 == PyDict_Contains(n, key)", so there is no "item". It turns out that the call "Py_DECREF(value)" goes in the code at the same place where "Py_DECREF(item)" used to be in older versions :)
So, after looking at newer code and applying the fix, I looked at 2.3.2 and my brain saw what it was expecting to see instead of what was. Being late afternoon on Friday made it worse... haha
-
Daniele Varrazzo
Oh, I see what you mean. I've checked if the error was introduced in the commit 99b3c723 (where the
PyDict_Containswas introduced) but it's been instead in psycopg since ever... It was probably in psycopg 1 too.Thank you again for the catch, and it was just in time for the release of 2.4.3: great!