In unicodeobject.c and stringlib aligned addresses and sizes are used for optimization. pointer->integer and implicit integer->integer conversions may overflow or underflow on platforms with sizeof(size_t) != sizeof(void *) or sizeof(size_t) != sizeof(int). The proposed patch fixes such unsafe things in unicodeobject.c, stringlib and some other files.

There are still a few unsafe places in libffi, but in this library Py_uintptr_t nor uintptr_t are not available.

If we're worrying about undefined behaviour, it looks like recent optimizations have *introduced* new undefined behaviour in the form of strict aliasing violations.  E.g., from ascii_decode:

    unsigned long value = *(const unsigned long *) _p;

(here _p has type const char *).  This should really be fixed;  compilers are known to make optimizations based on the assumption that this sort of undefined behaviour doesn't occur.

> This should really be fixed;  compilers are known to make optimizations 
> based on the assumption that this sort of undefined behaviour doesn't
> occur.

Doesn't the compiler have all the necessary information here? It's not like a subroutine is called.
(also, I'm not sure what optimization it could actually make)

> (also, I'm not sure what optimization it could actually make)

Me neither, but it doesn't seem safe to assume that no compiler will take advantage of this.  I don't want to start guessing what compilers might or might not do;  it would be much better simply to stick to valid C where possible.

N.B. This could probably be fixed without affecting performance by using the usual union trick.  (IIUC, that trick was technically still undefined behaviour for a while, but was eventually made legal by C99 + TC3.)  As far as I know there aren't any instances of compilers causing problems with that construct.

> N.B. This could probably be fixed without affecting performance by
> using the usual union trick.

How would it work? We would have to add various unions to the
PyUnicode_Object definition?

> How would it work? We would have to add various unions to the
> PyUnicode_Object definition?

No, you'd just need a temporary union defined in unicodeobject.c that would look something like:

typedef union { unsigned long v; char s[SIZEOF_LONG]; } U;

(with better choices of names).  Python/dtoa.c does a similar thing to read / write the pieces of a C double using integers safely.

I'll see if I can come up with a patch, and open a new issue for it (since I've successfully derailed this issue from its original topic)

> If we're worrying about undefined behaviour, it looks like recent optimizations have *introduced* new undefined behaviour in the form of strict aliasing violations.  E.g., from ascii_decode:
> 
>     unsigned long value = *(const unsigned long *) _p;
> 
> (here _p has type const char *).

I don't see what the undefined behavior. Can you specify exactly the
item of the Standard, in which it is mentioned?

>   This should really be fixed;  compilers are known to make optimizations based on the assumption that this sort of undefined behaviour doesn't occur.

I don't know how else you can rewrite it, without destroying completely
the effect of optimization.

In any case, I don't think that the original patch introduces some new
undefined behavior.

> I don't see what the undefined behavior. Can you specify exactly the
> item of the Standard, in which it is mentioned?

It's C99 section 6.5, paragraph 7:  "An object shall have its stored value accessed only by an lvalue expression that has one of the following types ...".  It's the dereferencing of the pointer that's the problem: that's accessing a stored value of type 'char' by an lvalue expression that has type 'unsigned long'.

> In any case, I don't think that the original patch introduces some new
> undefined behavior.

Ah;  were the strict aliasing problems already there before the patch?  I didn't check.

> Ah;  were the strict aliasing problems already there before the patch?  I didn't check.

Please open separate issue for this.

Please, Mark or some other C expert, can you do a code review for the patch?

Perhaps the three new macros should be made available in a .h file?
(in which case they should be private, i.e. start with _).

> Perhaps the three new macros should be made available in a .h file?
> (in which case they should be private, i.e. start with _).

Perhaps. There are similar macros in other files (Include/objimpl.h, 
Objects/obmalloc.c, Python/pyarena.c, Modules/expat/xmlparse.c).

> Perhaps the three new macros should be made available in a .h file?

Good idea. Maybe pymacros.h? These macros need to be documented (with a comment in the .h file)

Well, here is a new patch. The five new macros moved to pymacros.h and used in more files.

Apologies; I got distracted from the main point of this issue with the strict aliasing stuff, and then it fell off the to-do list.  Unassigning;  Antoine or Victor, do you want to take this?

Mark, please open a new discussion, so we don't lose it and that was the place for discussion.

Thanks for the patch! These macros will be useful.

New changeset 99112b851b25 by Antoine Pitrou in branch 'default':
Issue #15144: Fix possible integer overflow when handling pointers as integer values, by using Py_uintptr_t instead of size_t.
http://hg.python.org/cpython/rev/99112b851b25

Committed in 3.3(.1).

> Mark, please open a new discussion.

Done:  issue 15992.