Problem of using malloc() without including stdlib.h

[Anthony Petrov]

Hi Jonathan,

Let's consider e.g. 
src/share/native/sun/awt/splashscreen/java_awt_SplashScreen.c mentioned 
by you below.

It #includes splashscreen_impl.h. This file #includes 
splashscreen_config.h. And finally, 
src/solaris/native/sun/awt/splashscreen/splashscreen_config.h #includes 
the stdlib.h explicitly.

So, do you still see any issues with java_awt_SplashScreen.c?

Could you please go through your list and verify carefully whether these 
files actually #include stdlib.h or not?

--
best regards,
Anthony

On 2/3/2012 12:47 PM, Jonathan Lu wrote:
> 
> Sorry for being absent from this thread so long, I just experienced 
> couple of days with very limited Internet access.
> 
> Actually the problem was firstly found on AIX platform, on which 
> sizeof(malloc(1)) without including <stdlib.h> will always give 4 
> instead of 8. The symptom was runtime segment error not compiling time 
> warnings, and I suspect there're more failures which were not covered in 
> my application.
> 
> I've checked the build log of Linux64 platform, as Phil mentioned 
> there's also no such warnings except for the hotspot diagnostic ones. 
> One reason might be that some code inlcudes "malloc.h" instead of 
> <stdlib.h>, there's one 'extern' definition in malloc.h on Linux 64 
> platform, but I do not see one on AIX platform. I've also tried to trace 
> the indirect reference relation ship for all the .c/.cpp files using 
> malloc(), still cannot determine a explicit definition of following 
> files. pls fix me if anything wrong, because the list is produced 
> manually :(
> 
> ./src/share/native/sun/awt/splashscreen/splashscreen_png.c
> ./src/share/native/sun/awt/splashscreen/java_awt_SplashScreen.c
> ./src/share/native/sun/awt/splashscreen/splashscreen_gif.c
> ./src/share/native/sun/awt/splashscreen/splashscreen_impl.c
> ./src/share/native/sun/awt/image/dither.c
> ./src/share/native/sun/font/layout/KernTable.cpp
> ./src/share/native/sun/java2d/cmm/lcms/cmserr.c
> ./src/share/native/com/sun/media/sound/PlatformMidi.c
> ./src/share/bin/jli_util.c
> ./src/share/back/debugInit.c
> ./src/share/demo/jvmti/hprof/hprof_init.c
> ./src/share/demo/jvmti/hprof/hprof_table.c
> ./src/share/demo/jvmti/hprof/hprof_util.c
> ./src/windows/native/java/lang/ProcessImpl_md.c
> ./src/windows/native/java/lang/java_props_md.c
> ./src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c
> ./src/windows/native/java/io/dirent_md.c
> ./src/windows/native/java/io/io_util_md.c
> ./src/windows/native/sun/awt/splashscreen/splashscreen_sys.c
> ./src/windows/native/sun/tracing/dtrace/jvm_symbols_md.c
> ./src/windows/native/sun/windows/awt_new.cpp
> ./src/windows/native/sun/windows/CmdIDList.cpp
> ./src/windows/native/sun/nio/ch/DatagramDispatcher.c
> ./src/windows/native/sun/nio/ch/SocketDispatcher.c
> ./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_MidiIn.cpp
> ./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_Ports.c
> ./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_DirectSound.cpp
> ./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_Util.c
> ./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_MidiOut.c
> ./src/solaris/native/sun/java2d/opengl/GLXSurfaceData.c
> ./src/solaris/native/sun/java2d/x11/XRBackendNative.c
> ./src/solaris/native/sun/java2d/x11/X11SurfaceData.c
> ./src/solaris/native/sun/java2d/x11/X11TextRenderer_md.c
> ./src/solaris/native/sun/java2d/x11/X11Renderer.c
> 
> For a simple HelloWorld.c, if we use malloc() without including 
> stdlib.h, it will give warnings like "warning: incompatible implicit 
> declaration of built-in function ‘malloc’". No such warning were found 
> from the build log for above files, so I guess there must be some 
> headers has got indirect including or declaration of malloc.h, but I'm 
> worried because I did not see one via manual search. It indeed sounds 
> reasonable to me that if no warnings are produced and the compilation 
> runs OK, we may just leave the code untouched, but it will improve 
> portability if we can explicitly includes standard C library headers in 
> the right place (of cause, the 'right' place is also something need to 
> be discussed), right?
> 
> And for the 3rd party libraries, for those whose code has been shipped 
> with OpenJDK source, they seem OK to me since malloc() has either been 
> included or redefined. But for some dependency libraries of compilation, 
> such as X11 dev, are they also trustworthy?
> 
> Best regards!
> - Jonathan
> 
> On 01/19/2012 02:31 AM, Kelly O'Hair wrote:
>> A webrev and a code review from the appropriate teams is indeed needed.
>>
>> -kto
>>
>> On Jan 18, 2012, at 9:56 AM, Phil Race wrote:
>>
>>> Its arguable, whether harmless or not, that each file needs to 
>>> include it.
>>> Its not unreasonable for an area of the code to have a header file 
>>> such as "awt.h"
>>> that is supposed to be the one that's included by the other files in 
>>> that area of
>>> the code, and which takes care of common includes. jni_util.h is not 
>>> necessarily it.
>>> There isn't a need for every file to include that.
>>> Also many files are 3rd party libs and I don't like editing those as 
>>> the changes
>>> really belong upstream.
>>> So a one size fits all approach might be the answer but I'd want to 
>>> make sure
>>> of that first.
>>>
>>> So I'd like to see the list of files that generate actual warnings as 
>>> well as the list
>>> of files that reference malloc but don't include stdlib.h.
>>>
>>> We are well aware that returning int as a default is bad in 64 bit .. 
>>> I'd expect
>>> instant death so I'd like to see those actual warnings rather than 
>>> just the
>>> theoretical ones.
>>>
>>> My grep of a current JDK 8 build log for 64 bit Linux shows the only 
>>> malloc warnings
>>> are in hotspot management code. So I am waiting for the proof of the 
>>> real problem
>>>
>>> And I can speak for 2d, and if there's 2D files touched I would like 
>>> to see any changes
>>> to those files, and the reasoning discussed on 2d-dev ..
>>>
>>> -phil.
>>>
>>> On 1/18/2012 8:26 AM, Kelly O'Hair wrote:
>>>> On Jan 18, 2012, at 12:19 AM, Jonathan Lu wrote:
>>>>
>>>>> Hi core-libs-dev,
>>>>>
>>>>> I found that for some native code of OpenJDK code base, malloc() is 
>>>>> used without including header file stdlib.h, such as following files,
>>>>> ./src/solaris/native/sun/java2d/opengl/GLXSurfaceData.c
>>>>> ./src/solaris/native/sun/java2d/x11/XRBackendNative.c
>>>>> ....
>>>>>
>>>>> I assume that there's no hacking tricks involved here, right? 
>>>>> because this may cause problem for some C compilers, which assumes 
>>>>> 'int' as the default return type of a function if it cannot find 
>>>>> the function's declaration during compiling. Under such a 
>>>>> condition, actual return result of type 'void*' from malloc() will 
>>>>> be converted to 'int', which may result in truncated pointers in 
>>>>> 64bit platforms. If the application tries to dereference such a 
>>>>> broken pointer, error will occur.
>>>>>
>>>>> Indeed I found some indirect includes of stdlib.h, but there're 
>>>>> still some I do not see a stdlib.h get included from any of  the 
>>>>> direct/indirect included headers. I think in order to fix this 
>>>>> problem, two approaches may be considered here,
>>>>> a) add "#include<stdlib.h>" to every missing .c file.
>>>>> b) add "#include<stdlib.h>" to a commonly  referenced header file, 
>>>>> such as jni_util.h. but it would not be easy to find such a file 
>>>>> for all and creating one is the same as approach a).
>>>>>
>>>> I suggest a)  It should be harmless and is the right thing to do.
>>>>
>>>> It's been a while since I studied the C standard, but I vaguely 
>>>> recall that there was another standard C include file
>>>> that would cause the malloc() prototype declaration to show up.
>>>> Or maybe it wasn't a standard one.  In any case, I think your a) 
>>>> approach is correct and I don't see much a need
>>>> for detailed discussions on this, as long as it builds correctly 
>>>> with no warnings on all platforms. It should be fine and correct.
>>>> That's my 2 cents.
>>>>
>>>> -kto
>>>>
>>>>> But both methods need to change many files, any other ideas about 
>>>>> how to fix it more elegantly?
>>>>>
>>>>> Thanks and best regards!
>>>>> - Jonathan
>>>>>
>