Re: RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
On Tue, 23 Apr 2024 07:51:28 GMT, Matthias Baesken wrote: > In the hashN usages of readCen from zip_util.c we see a lot of signed integer > overflows. > For example in the java/util jtreg tests those are easily reproducable when > compiling with -ftrapv (clang/gcc toolchains). > While those overflows never seem to cause crashes or similar errors, they are > unwanted because > signed integer overflows in C cause undefined behavior. > See > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html >> >> For signed integers, the result of overflow in C is in principle undefined, >> meaning that anything whatsoever could happen. >> Therefore, C compilers can do optimizations that treat the overflow case >> with total unconcern. > > So we might still get unwanted results (maybe bad/strange hashing, depending > on compiler and optimization level). > > Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the > issue : > # Problematic frame: > # C [libzip.dylib+0x6362] hashN+0x32 > # > > Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free > space=1021k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libzip.dylib+0x6362] hashN+0x32 > C [libzip.dylib+0x5d5e] readCEN+0xd2e > C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 > V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, > JavaThread*)+0x3e > V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, > char const*, stat const*, bool, bool)+0x6c > V [libjvm.dylib+0x543833] > ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 > V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b > V [libjvm.dylib+0x92602a] init_globals()+0x3a > V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 > V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 > C [libjli.dylib+0x4483] JavaMain+0x123 > C [libjli.dylib+0x7529] ThreadJavaMain+0x9 > C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 > C [libsystem_pthread.dylib+0x2443] thread_start+0xf Hello Matthias, the tests completed a couple of hours back and no failures related to this change have been observed. Thank you for waiting. - PR Comment: https://git.openjdk.org/jdk/pull/18908#issuecomment-2077527559
Re: RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
On Tue, 23 Apr 2024 07:51:28 GMT, Matthias Baesken wrote: > In the hashN usages of readCen from zip_util.c we see a lot of signed integer > overflows. > For example in the java/util jtreg tests those are easily reproducable when > compiling with -ftrapv (clang/gcc toolchains). > While those overflows never seem to cause crashes or similar errors, they are > unwanted because > signed integer overflows in C cause undefined behavior. > See > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html >> >> For signed integers, the result of overflow in C is in principle undefined, >> meaning that anything whatsoever could happen. >> Therefore, C compilers can do optimizations that treat the overflow case >> with total unconcern. > > So we might still get unwanted results (maybe bad/strange hashing, depending > on compiler and optimization level). > > Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the > issue : > # Problematic frame: > # C [libzip.dylib+0x6362] hashN+0x32 > # > > Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free > space=1021k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libzip.dylib+0x6362] hashN+0x32 > C [libzip.dylib+0x5d5e] readCEN+0xd2e > C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 > V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, > JavaThread*)+0x3e > V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, > char const*, stat const*, bool, bool)+0x6c > V [libjvm.dylib+0x543833] > ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 > V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b > V [libjvm.dylib+0x92602a] init_globals()+0x3a > V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 > V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 > C [libjli.dylib+0x4483] JavaMain+0x123 > C [libjli.dylib+0x7529] ThreadJavaMain+0x9 > C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 > C [libsystem_pthread.dylib+0x2443] thread_start+0xf Hi Lutz and Martin, thanks for the reviews! Jaikiran, are you done with your additional tests ? - PR Comment: https://git.openjdk.org/jdk/pull/18908#issuecomment-2077026022
Re: RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
On Tue, 23 Apr 2024 07:51:28 GMT, Matthias Baesken wrote: > In the hashN usages of readCen from zip_util.c we see a lot of signed integer > overflows. > For example in the java/util jtreg tests those are easily reproducable when > compiling with -ftrapv (clang/gcc toolchains). > While those overflows never seem to cause crashes or similar errors, they are > unwanted because > signed integer overflows in C cause undefined behavior. > See > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html >> >> For signed integers, the result of overflow in C is in principle undefined, >> meaning that anything whatsoever could happen. >> Therefore, C compilers can do optimizations that treat the overflow case >> with total unconcern. > > So we might still get unwanted results (maybe bad/strange hashing, depending > on compiler and optimization level). > > Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the > issue : > # Problematic frame: > # C [libzip.dylib+0x6362] hashN+0x32 > # > > Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free > space=1021k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libzip.dylib+0x6362] hashN+0x32 > C [libzip.dylib+0x5d5e] readCEN+0xd2e > C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 > V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, > JavaThread*)+0x3e > V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, > char const*, stat const*, bool, bool)+0x6c > V [libjvm.dylib+0x543833] > ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 > V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b > V [libjvm.dylib+0x92602a] init_globals()+0x3a > V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 > V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 > C [libjli.dylib+0x4483] JavaMain+0x123 > C [libjli.dylib+0x7529] ThreadJavaMain+0x9 > C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 > C [libsystem_pthread.dylib+0x2443] thread_start+0xf Undefined behavior should always get fixed. Thanks for doing it! - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18908#pullrequestreview-2022189272
Re: RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
On Tue, 23 Apr 2024 07:51:28 GMT, Matthias Baesken wrote: > In the hashN usages of readCen from zip_util.c we see a lot of signed integer > overflows. > For example in the java/util jtreg tests those are easily reproducable when > compiling with -ftrapv (clang/gcc toolchains). > While those overflows never seem to cause crashes or similar errors, they are > unwanted because > signed integer overflows in C cause undefined behavior. > See > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html >> >> For signed integers, the result of overflow in C is in principle undefined, >> meaning that anything whatsoever could happen. >> Therefore, C compilers can do optimizations that treat the overflow case >> with total unconcern. > > So we might still get unwanted results (maybe bad/strange hashing, depending > on compiler and optimization level). > > Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the > issue : > # Problematic frame: > # C [libzip.dylib+0x6362] hashN+0x32 > # > > Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free > space=1021k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libzip.dylib+0x6362] hashN+0x32 > C [libzip.dylib+0x5d5e] readCEN+0xd2e > C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 > V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, > JavaThread*)+0x3e > V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, > char const*, stat const*, bool, bool)+0x6c > V [libjvm.dylib+0x543833] > ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 > V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b > V [libjvm.dylib+0x92602a] init_globals()+0x3a > V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 > V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 > C [libjli.dylib+0x4483] JavaMain+0x123 > C [libjli.dylib+0x7529] ThreadJavaMain+0x9 > C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 > C [libsystem_pthread.dylib+0x2443] thread_start+0xf Thank you Matthias, for this change. Lance has run some internal CI tests and they have come back fine. I am in the process of running some more CI tests with this change and I should have the results, very likely by tomorrow. Please wait for those results before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/18908#issuecomment-2074986166
Re: RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
On Tue, 23 Apr 2024 07:51:28 GMT, Matthias Baesken wrote: > In the hashN usages of readCen from zip_util.c we see a lot of signed integer > overflows. > For example in the java/util jtreg tests those are easily reproducable when > compiling with -ftrapv (clang/gcc toolchains). > While those overflows never seem to cause crashes or similar errors, they are > unwanted because > signed integer overflows in C cause undefined behavior. > See > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html >> >> For signed integers, the result of overflow in C is in principle undefined, >> meaning that anything whatsoever could happen. >> Therefore, C compilers can do optimizations that treat the overflow case >> with total unconcern. > > So we might still get unwanted results (maybe bad/strange hashing, depending > on compiler and optimization level). > > Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the > issue : > # Problematic frame: > # C [libzip.dylib+0x6362] hashN+0x32 > # > > Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free > space=1021k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libzip.dylib+0x6362] hashN+0x32 > C [libzip.dylib+0x5d5e] readCEN+0xd2e > C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 > V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, > JavaThread*)+0x3e > V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, > char const*, stat const*, bool, bool)+0x6c > V [libjvm.dylib+0x543833] > ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 > V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b > V [libjvm.dylib+0x92602a] init_globals()+0x3a > V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 > V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 > C [libjli.dylib+0x4483] JavaMain+0x123 > C [libjli.dylib+0x7529] ThreadJavaMain+0x9 > C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 > C [libsystem_pthread.dylib+0x2443] thread_start+0xf LGTM. - Marked as reviewed by lucy (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18908#pullrequestreview-2019585061
RFR: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
In the hashN usages of readCen from zip_util.c we see a lot of signed integer overflows. For example in the java/util jtreg tests those are easily reproducable when compiling with -ftrapv (clang/gcc toolchains). While those overflows never seem to cause crashes or similar errors, they are unwanted because signed integer overflows in C cause undefined behavior. See https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html > > For signed integers, the result of overflow in C is in principle undefined, > meaning that anything whatsoever could happen. > Therefore, C compilers can do optimizations that treat the overflow case with > total unconcern. So we might still get unwanted results (maybe bad/strange hashing, depending on compiler and optimization level). Compilation with -ftrapv causes/triggers this SIGILL on macOS showing the issue : # Problematic frame: # C [libzip.dylib+0x6362] hashN+0x32 # Stack: [0x7c496000,0x7c596000], sp=0x7c5957e0, free space=1021k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) C [libzip.dylib+0x6362] hashN+0x32 C [libzip.dylib+0x5d5e] readCEN+0xd2e C [libzip.dylib+0x4ee0] ZIP_Put_In_Cache0+0x160 V [libjvm.dylib+0x544b1e] ClassLoader::open_zip_file(char const*, char**, JavaThread*)+0x3e V [libjvm.dylib+0x543fec] ClassLoader::create_class_path_entry(JavaThread*, char const*, stat const*, bool, bool)+0x6c V [libjvm.dylib+0x543833] ClassLoader::setup_bootstrap_search_path_impl(JavaThread*, char const*)+0xf3 V [libjvm.dylib+0x54819b] classLoader_init1()+0x1b V [libjvm.dylib+0x92602a] init_globals()+0x3a V [libjvm.dylib+0x12b3b74] Threads::create_vm(JavaVMInitArgs*, bool*)+0x314 V [libjvm.dylib+0xa848f4] JNI_CreateJavaVM+0x64 C [libjli.dylib+0x4483] JavaMain+0x123 C [libjli.dylib+0x7529] ThreadJavaMain+0x9 C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0 C [libsystem_pthread.dylib+0x2443] thread_start+0xf - Commit messages: - JDK-8330615 Changes: https://git.openjdk.org/jdk/pull/18908/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18908&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330615 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18908.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18908/head:pull/18908 PR: https://git.openjdk.org/jdk/pull/18908
