Author Oren Milman
Recipients Oren Milman, ezio.melotti, mark.dickinson, serhiy.storchaka, terry.reedy
Date 2016-10-15.23:43:42
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1476575026.55.0.10870549308.issue15988@psf.upfronthosting.co.za>
In-reply-to
Content
Sorry for taking so long to come up with a patch..

------------ proposed changes ------------
    1. in Python/getargs.c in seterror, the following patches:
        - add a parameter 'p_format_code' - a pointer to the format code
          whose conversion failed.
        - replace the error message of the OverflowError with
          '{fname}() argument out of range' in case all of the following are
          true:
            * PyErr_Occurred() and the exception is an OverflowError.
            * The PyArg_* function received a format ending with a ':<fname>'
              (i.e. seterror's fname argument is not NULL).
            * The error occurred during a conversion to a C integer type
              which might overflow, i.e. one of the format codes 'bhilLn'.

      With these patches, inconsistent messages could often be fixed by merely
      appending ':<fname>' to the format argument in a call to a PyArg_*
      function.

      Furthermore, Some inconsistent messages are actually fixed by these
      patches alone.
      That is because there are already many calls to PyArg_* functions with a
      format ending with ':<fname>'. Some of them are followed by more
      checks of the parsed arguments, which might result in raising an
      OverflowError/ValueError with an inconsistent error message.
      (e.g. in Modules/itertoolsmodule.c, product_new already calls
      PyArg_ParseTupleAndKeywords with the format "|n:product", so with these
      patches, in case we do 'itertools.product('a', repeat=1 << 1000)', the
      error message would be 'product() argument out of range').

      Also, ISTM this patch is helpful, regardless of this issue (#15988).
      In case a PyArg_* function raises an OverflowError (because a conversion
      to some C integer type failed), knowing which function called the
      PyArg_* function would probably prove more helpful to a user (than
      knowing which C type Python failed to convert to, and whether the
      conversion failed because the number to convert was too large or too
      small).

      I decided to put the patch inside seterror, because (aside from its
      name,) it is always called after a failure of convertitem (which is the
      only caller of convertsimple).

    2. in various files:
        - change some OverflowError/ValueError messages for more clarity, e.g.
          replace 'unsigned byte integer is greater than maximum' with
          'Python int too large to convert to C unsigned char'.
        - add code to "catch" OverflowError/ValueError errors, to prevent
          raising inconsistent error messages
        - append ':<fname>' to formats passed to PyArg_* functions, to utilize
          the first proposed change (to automatically "catch"
          OverflowError/ValueError errors).

    3. in Lib/tests - I was already writing tests for my patches, so I guessed
      I should add some of them (the basic boundary checks) to Lib/tests (in
      case they didn't already exist). I ran into some issues here:
        - test_pickle - I didn't manage to create an int large enough to test
          my patch for save_long in Modules/_pickle.c (and so I didn't add any
          test to test_pickle).
        - test_stat - I didn't find a way to determine the size of mode_t on
          the current platform, except for Windows (so the test I added is
          incomprehensive).

    4. in Objects/longobject.c, make some messages more helpful (as mentioned
      in the last message I posted here).


------------ diff ------------
The proposed patches diff file is attached.


------------ tests ------------
I wrote an ugly script to test my patches, and ran it on my Windows 10 and
Ubuntu 16.04 64-bit VM. The script is attached, but it might fail on another
platform.
  - Note that the script I wrote has (among others) a test for
    'select.devpoll'. I couldn't run it, as I don't have a Solaris machine,
    but the test is identical to that of 'select.poll', so it should run
    smoothly on a Solaris machine. Theoretically.
(Each test in my script verifies the exception's error message, and not only
which exception was raised, so ISTM these kind of tests don't fit in
Lib/tests. Please let me know if you think otherwise.)

In addition, I ran 'python_d.exe -m test -j0' (on my 64-bit Windows 10 and
Ubuntu VM) with and without the patches, and got quite the same output. (That
also means my new tests in various files in Lib/tests have passed.)


##############################################################################

Note that the inconsistent messages my patches fix are only those I found
while going over each 'PyExc_OverflowError' in the codebase (in *.h and *.c
files).
However, I would probably find another bunch of inconsistent messages when I
go over each 'PyExc_ValueError' in the codebase.

I would be happy to do that, but I am afraid I won't have time to (at least)
until next June.
History
Date User Action Args
2016-10-15 23:43:51Oren Milmansetrecipients: + Oren Milman, terry.reedy, mark.dickinson, ezio.melotti, serhiy.storchaka
2016-10-15 23:43:46Oren Milmansetmessageid: <1476575026.55.0.10870549308.issue15988@psf.upfronthosting.co.za>
2016-10-15 23:43:46Oren Milmanlinkissue15988 messages
2016-10-15 23:43:46Oren Milmancreate