Re: Built XWin on mingw - with patches

[Jon TURNEY <jon.turney@dronecode.org.uk>]

On 10/11/2011 16:50, Ryan Pavlik wrote:
> On Mon, Nov 7, 2011 at 12:10 PM, Jon TURNEY wrote:
>> 0009-os-utils.c-Use-winxp-or-better-for-Winsock-API.patch
>>
>> I am a bit unclear why this is needed, surely the winsock API predates XP?
>> It might be better to add this define to CFLAGS rather than to start
>> sprinkling it around source files as needed?
> 
> Yes, but one of the calls in that file uses a part of the winsock API
> introduced in XP - getaddrinfo and freeaddrinfo.
> http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/w32api/include/ws2tcpip.h?rev=1.12&content-type=text/x-cvsweb-markup&cvsroot=src

In my build testing, this only seems to be a problem if you explicitly
./configure the X server with --enable-ipv6, and in that case other build
problems exist as well (No inet_pton/inet_ntop, inclusion of ws2tcpip.h is needed)

(Ideally, if you were to ./configure with CFLAGS=-D_WIN32_WINNT=0x0501, IPv6
should be auto-detected by ./configure and build successfully.  But this
auto-detection doesn't work, because AC_CHECK_FUNC(getaddrinfo) fails because
the test program generated doesn't prototype getaddrinfo, so it doesn't look
for it with stdcall mangling...)

So it seems there are a couple of generic problems here, so I'm not sure
fixing it like this is the right thing to do.

>> 0027-dix-registry.c-Free-old-memory-upon-realloc-failure.patch
>>
>> Interesting.
>>
>> It would probably be useful to quote the language from the appropriate
>> standard which describes the behavior of realloc() in this error case in the
>> comment.
>>
>> I don't think this change is fully correct however.  If the realloc'ed size
>> is 0, realloc() may return NULL, but the previously allocated memory has
>> been freed.  Perhaps you need to check if errno has been set by realloc to
>> distinguish these two cases?
>>
>> Did you notice this by inspection or actually have a problem caused by this
>> code? Have you audited the rest of the xserver code for this class of error?
> 
> Good point. I found this with cppcheck - a static analysis tool that,
> despite its name, is useful for C code as well. There were other
> issues it mentioned in the xserver code, but I didn't get to any of
> the others yet. In any case, it's a completely orthogonal patch. Might
> be useful for someone more familiar with the code to run cppcheck and
> address the issues.

Since it's outside my area of expertise to do a good review, I'd suggest you
post this patch (when you have it in a correct form) directly to xorg-devel.

>> 0041-configure.ac-mingw-doesn-t-have-setuid-either.patch
>>
>> Use whitespace consistently with the context, please
> 
> Oops - will correct.

Looking at this again, I'm a bit puzzled by the comment "Fixes having to pass
this flag for a successful MinGW build"

I can understand adding MinGW to the set of targets which don't have setuid
binaries, but I'm not sure how the MinGW build can fail if this flag isn't
supplied: INSTALL_SETUID appears to only apply to installing the Xorg DDX.

Is the real bug here that the test immediately below this assumes that we are
not cross-compiling?  Should the test check for cross-compiling and assume
setuid binaries are possible unless it's on the list of excluded targets?

-- 
Jon TURNEY
Volunteer Cygwin/X X Server maintainer

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/