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.
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
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
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; \
_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.
(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