classification
Title: PyUnicode_FromFormatV() bugs with "%" and "%%" format strings
Type: behavior Stage:
Components: Interpreter Core, Unicode Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, haypo, ysj.ray
Priority: normal Keywords: patch

Created on 2011-01-05 00:46 by haypo, last changed 2011-03-03 08:34 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
pyunicode_fromformatv-2.patch haypo, 2011-01-05 01:27
parse_format_flags.patch haypo, 2011-02-21 23:47
Messages (10)
msg125387 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-01-05 00:46
Steps 1 and 3 of PyUnicode_FromFormatV() doesn't handle the format string "%%" correctly. The loop responsible to skip the precision moves outside the format string, and the function will then read uninitialized memory. The loop:

             while (*++f && *f != '%' && !Py_ISALPHA((unsigned)*f))
                 ;

This is another issue:

    for (f = format; *f; f++) {
         if (*f == '%') {
             if (*(f+1)=='%')
                 continue;
    ...

continue only skips the first %: with "%%", the second % will be interpreted (and not escaped).

Attached patch fixes the issue, but I don't feal confortable with this ugly function, and I would appreciate a review :-) The patch adds unit tests.

I found the bug when trying to add new tests before trying to implement "%zi" format. I was first surprised that "%zi" (and %li and %lli) is not supported, but now I am surprised because I found bugs :-)
msg125392 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-01-05 01:27
Oh, my first patch was wrong: it failed on %c format (in zipimport with "%U%c%U"). The second version of the patch should be better.
msg125397 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-01-05 02:45
PyBytes_FromFormatV() has the same issue.
msg125722 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-01-07 22:28
> PyBytes_FromFormatV() has the same issue.

Oh no, it's ok, the function works as expected.
msg128961 - (view) Author: ysj.ray (ysj.ray) Date: 2011-02-21 13:30
Hi, haypo, Your patch seems cannot be applied cleanly on current py3k trunk. And after modified your patch, test_unicode.py runs into Segmentation fault. Is there something wrong or some changes which could influence this bug had been already made since the patch is worked out?


On the current trunk, I guess the bug could be fixed in a simpler way:

In step 1, before check '%%', check '%'(a string ends with '%') first. Then check '%%' and skip it.

The whole patch:

Index: Objects/unicodeobject.c
===================================================================
--- Objects/unicodeobject.c	(revision 88453)
+++ Objects/unicodeobject.c	(working copy)
@@ -750,8 +750,12 @@
      * result in an array) */
     for (f = format; *f; f++) {
          if (*f == '%') {
-             if (*(f+1)=='%')
+             if (*(f+1)=='\0')
+                continue;
+             if (*(f+1)=='%') {
+                 f++;
                  continue;
+             }
              if (*(f+1)=='S' || *(f+1)=='R' || *(f+1)=='A')
                  ++callcount;
              while (Py_ISDIGIT((unsigned)*f))


After applying this small patch and tests in your patch, test_unicode.py can passed.
msg129013 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-02-21 23:47
Well, the main problem is that there are 3 different codes to parse the format string, and each code is different... Attached patch factorizes the code: create one subfunction parse_format_flags(). It fixes also this issue and prepares the work to fix #10831.
msg129038 - (view) Author: ysj.ray (ysj.ray) Date: 2011-02-22 07:24
> Well, the main problem is that there are 3 different codes to parse the format string, and each code is different... Attached patch factorizes the code: create one subfunction parse_format_flags(). It fixes also this issue and prepares the work to fix #10831.


Sounds nice! Maybe several related issues can also use this, like #7330.
msg129836 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-03-02 00:12
Fixed in Python 3.3 (r88702+r88703).

I don't really want to backport it to 3.2, 3.1 or 2.7, because the patch is complex, changes a very important function, and the fixed bug (don't crash on invalid format strings) is minor. If you would like the fix into in old release, please reopen the issue and tell me why.
msg129931 - (view) Author: ysj.ray (ysj.ray) Date: 2011-03-03 03:51
Hi, haypo, would you mind modify your newly added parse_format_flags() function so that it can diff the precision value of '%.0s' and '%s'(Currently both of them return precision as 0)? Because if used with string formatters(%s, %R, %S, %A, ...), they should be very different. And so I can work on issue7330 with it.

Just return precision as -1 to indicate no precision is designated could work.
msg129938 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-03-03 08:34
> Hi, haypo, would you mind modify your newly added parse_format_flags()
> function so that it can diff the precision value of '%.0s' and
> '%s'(Currently both of them return precision as 0)?

You should update your patch attached to #7330. I hope that your patch will be simpler with parse_format_flags() than before.
History
Date User Action Args
2011-03-03 08:34:27hayposetnosy: amaury.forgeotdarc, haypo, ysj.ray
messages: + msg129938
2011-03-03 03:51:17ysj.raysetnosy: amaury.forgeotdarc, haypo, ysj.ray
messages: + msg129931
2011-03-02 00:12:38hayposetstatus: open -> closed

messages: + msg129836
resolution: fixed
nosy: amaury.forgeotdarc, haypo, ysj.ray
2011-02-22 07:24:09ysj.raysetnosy: amaury.forgeotdarc, haypo, ysj.ray
messages: + msg129038
2011-02-21 23:47:26hayposetfiles: + parse_format_flags.patch
nosy: amaury.forgeotdarc, haypo, ysj.ray
messages: + msg129013
2011-02-21 13:31:33ysj.raysetnosy: amaury.forgeotdarc, haypo, ysj.ray
type: behavior
2011-02-21 13:30:39ysj.raysetnosy: + ysj.ray
messages: + msg128961
2011-01-07 22:28:58hayposetmessages: + msg125722
2011-01-05 03:10:56hayposetfiles: - pyunicode_fromformatv.patch
2011-01-05 02:45:49hayposetmessages: + msg125397
2011-01-05 01:27:34hayposetfiles: + pyunicode_fromformatv-2.patch

messages: + msg125392
2011-01-05 00:46:25haypocreate