Bug 17968

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 

                    memcpy(pdata,NULL,0) 

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); \
}

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.

(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)); \
    _iceConn->send_sequence++

Thus _pData could be set to NULL.

(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

Thanks for the patch.

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