| msg71712 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2008-08-22 01:02 |
| I'm getting a compiler warning on Linux AMD64. It's most probably an issue related to 64bit builds, because size_t > uint_t on 64bit systems. Such warnings may hide overflow issues. gcc -pthread -c -fno-strict-aliasing -g -Wall -Wstrict-prototypes -I. -IInclude -I./Include -DPy_BUILD_CORE -o Objects/obmalloc.o Objects/obmalloc.c Objects/obmalloc.c: In function 'new_arena': Objects/obmalloc.c:529: warning: comparison is always false due to limited range of data type I was able to silence the compiler by declaring numarenas as size_t instead of uint. |
|
|
| msg71713 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-08-22 01:06 |
| I'm seeing a similar issue in py3k: Objects/bytesobject.c: In function '_PyBytes_FormatLong': Objects/bytesobject.c:3203: warning: comparison is always false due to limited range of data type |
|
|
| msg71772 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-08-22 19:51 |
| The warning in obmalloc.c was silenced in r65975 (trunk) and r65976 (py3k) |
|
|
| msg71820 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-08-23 22:42 |
| There is no patch here, so undoing the "needs review" flag. |
|
|
| msg71843 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-08-24 16:42 |
| I've fixed both compiler warnings in trunk and py3k branch. |
|
|
| msg71893 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-08-24 22:10 |
| I think the patch is fairly incomplete. "i" still stays at uint, so if numarenas would ever overflow uint, the loop would never reach numarenas, and all arenas would get discarded. Likewise, maxarenas still stays at uint, so when numarenas overflows uint, the additional arenas get discarded. The compiler was right observing that the condition is always false: this was a security check to prevent overflow, but on this specific system, overflow couldn't occur in the first place. If you want to silence the warning, try casting numarenas to size_t in the test only. If you want to properly remove the unnecessary test, put an ifdef around it, testing whether size_t is larger than uint. With the current code, it might be that you have disabled the security check: numarenas <= maxarenas will not occur anymore (since numarenas is wider than maxarenas); as a consequence, you then see the silent truncation later on instead of the exception. |
|
|
| msg71897 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-08-24 22:45 |
| Good analysis Martin! In my humble opinion it should be safe to limit the amount of arenas to UINT_MAX instead of PY_SIZE_MAX. 4,294,967,295 arenas should be more than sufficient for the next decade or two. Do you concur? |
|
|
| msg71909 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-08-25 04:50 |
| > In my humble opinion it should be safe to limit the amount of arenas to > UINT_MAX instead of PY_SIZE_MAX. 4,294,967,295 arenas should be more > than sufficient for the next decade or two. Do you concur? It is certainly reasonable to limit arenas to uint. Still, the test for SIZE_MAX must remain: it doesn't test whether numarenas overflows, but whether numarenas*sizeof(*arenas) overflows as a parameter for realloc. As the parameter for realloc is size_t (per C spec), the overflow test needs to use PY_SIZE_MAX. |
|
|
| msg72942 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-09-10 05:41 |
| Going with what Martin suggested, the attached patch reverts what Christian did and adds an #ifdef sizeof(uint) <= sizeof(uint) around the PY_SIZE_MAX check. Someone should obviously test on an AMD64 machine (I have a Core 2 Mac, but I don't know what flags I would need to pass to trigger the warning on my machine). |
|
|
| msg72943 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-09-10 05:53 |
| It doesn't work that way - you can't use sizeof() in the preprocessor. Instead, the pyconfig.h constants should be used. I'll try to look into this later today. |
|
|
| msg72968 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-09-10 15:43 |
| Here is my attempt for a patch (obmalloc.diff). Please review. |
|
|
| msg72994 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-09-10 22:40 |
| Patch looks good to me and Martin's analysis of the test being useless on a platform where size_t > uint_t makes sense. |
|
|
| msg73007 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-09-11 06:57 |
| I have now committed the new patch as r66383 and r66384 |
|
|