Bug 17968

Summary: patches to avoid gcc warnings for libSM + comment about potential segfault
Product: xorg Reporter: Peter Breitenlohner <peb>
Component: Lib/otherAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium Keywords: patch
Version: 7.3 (2007.09)   
Hardware: Other   
OS: All   
i915 platform: i915 features:
Description Flags
patch to avoid gcc warnings for libSM (V 1.1.0) none

Description Peter Breitenlohner 2008-10-08 01:08:34 UTC
Created attachment 19482 [details] [review]
patch to avoid gcc warnings for libSM (V 1.1.0)

Attached is a small patch to avoid gcc warnings for libSM (V 1.1.0)

IMPORTANT: While looking at the code I noticed a cause for potential (hypothetical) segmentation faults.

Routines in libSM (and libICE and probably others) use IceGetHeaderExtra to ensure there is enough room in the buffer. That is a macro defined in include/X11/ICE/ICEmsg.h with a code path that may set the buffer pointer to NULL.

However, these routines continue to store into that buffer without checking for this particular condition.

It may well be that the above mentioned code path is purely hypothetical. Nevertheless, defensive programming would require to check for this case and take appropriate action.
Comment 1 Hugo Mildenberger 2008-11-14 07:02:51 UTC
I stumbled upon the very same warning, causing libSM-1.1.0 failing to compile, if Gentoo's make option "stricter" was set:

 * sm_client.c:269: warning: null argument where non-null 
		             required (argument 2)
 * The die message:  poor code kills airplanes
Even though airplanes shouldn't be directly harmed by poor old
libSM (but maybe passenger seat's video screens), I too looked 
into src/sm_client.c:

269                 STORE_ARRAY8 (pData, 0, NULL);

This statement will be expand into something like 


As Peter said, this could trigger a segmentation fault,depending on implementation of memcpy and NULL-page protection.

But the question is, what the author really intended. It was 
probably not
       STORE_ARRAY8 (pData, 0, "");

as the patch assumes, because this too doesn't make much sense:

STORE_ARRAY_8 as defined in libSMint.h:

#define STORE_ARRAY8(_pBuf, _len, _array8) 
{ \
    STORE_CARD32 (_pBuf, _len); \
    memcpy (_pBuf, _array8, _len); \
    _pBuf += _len; \
    if (PAD64 (4 + _len)) \
        _pBuf += PAD64 (4 + _len); \
Comment 2 Peter Breitenlohner 2008-11-14 07:37:43 UTC
What's wrong with STORE_ARRAY8 (pData, 0, "") ? This stores a 4 byte length of zero followed by zero data bytes and 4 pad bytes.

I think that's exactly what the author intended and what is required by the protocol.

The protocol (xsmp.PS.gz) specifies for RegisterClient: If previous-ID is not valid, .... The client should then send a RegisterClient with a null previous-ID field.
Comment 3 Hugo Mildenberger 2008-11-14 11:04:11 UTC
(In reply to comment #2)

Effectively, that's true, provided pData is valid. But actually,
pData could be NULL.

Look at this statement above:

265  IceGetHeaderExtra (iceConn, _SmcOpcode, SM_RegisterClient,
266                     SIZEOF (smRegisterClientMsg), WORD64COUNT (extra),
267                     smRegisterClientMsg, pMsg, pData);

I just wondered how pData would get it's value, until I saw that IceGetHeaderExtra is not a function, but a macro, defined in /usr/X11R6/include/X11/ICE/ICEmsg.h:

#define IceGetHeaderExtra(_iceConn, _major, _minor, _headerSize, _extra, _msgType, _pMsg, _pData) \
    if ((_iceConn->outbufptr + \
        _headerSize + ((_extra) << 3)) > _iceConn->outbufmax) \
        IceFlush (_iceConn); \
    _pMsg = (_msgType *) _iceConn->outbufptr; \
    if ((_iceConn->outbufptr + \
        _headerSize + ((_extra) << 3)) <= _iceConn->outbufmax) \
        _pData = (char *) _pMsg + _headerSize; \
    else \
        _pData = NULL; \
    _pMsg->majorOpcode = _major; \
    _pMsg->minorOpcode = _minor; \
    _pMsg->length = ((_headerSize - SIZEOF (iceMsg)) >> 3) + (_extra); \
    _iceConn->outbufptr += (_headerSize + ((_extra) << 3)); \

Thus _pData could be set to NULL.
Comment 4 Peter Breitenlohner 2008-11-15 04:29:40 UTC
(In reply to comment #3)
> (In reply to comment #2)
> Effectively, that's true, provided pData is valid. But actually,
> pData could be NULL.
> ......
> Thus _pData could be set to NULL.

Exactly what I said in my original comment
Comment 5 Paulo C├ęsar Pereira de Andrade 2008-11-28 23:00:55 UTC
Thanks for the patch.

Committed (by making minor changes to apply in current
sources) as 1dcf5502137efe36d01b30169d4387438ad47be0

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.