#143 Possible bug: less-than-zero comparison of an unsigned int

[vstrinski]

While running log4cpp-1.1.2rc3 through a static code analyzer (required for our company) I got a defect that is most likely a bug.

In RemoteSyslogAppender.cpp line 118:
if ((_socket = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
results in checking if the unsigned int _socket is less than zero - which it will never be.

A very sneaky tiny little bug :)

Thanks again to all developers for their outstanding work!

p.s. The static code analyer software is Coverity if that matters.

[Alexander Perepelkin]

Thanks for the report, but POSIX socket() function may return negative:

       On success, a file descriptor for the new socket is returned.  On
       error, -1 is returned, and errno is set appropriately.

See http://man7.org/linux/man-pages/man2/socket.2.html, for example

[vstrinski]

Hi Alexander - first thanks for looking into this issue!

To the question - in the case of error it doesn't return "any negative value" - it returns a specific value (negetive one to be exact).

I think the solution would be to compare the result with -1 as in this example:
http://man7.org/linux/man-pages/man3/getaddrinfo.3.html

for (rp = result; rp != NULL; rp = rp->ai_next) {
    sfd = socket(rp->ai_family, rp->ai_socktype,
 rp->ai_protocol);
    if (sfd == -1) { ...

Even though it is not optimal to compare an unsigned int with a negative value the compiler will do the right thing at least.

Proof of concept:

Just for the sake of this experiment I moved the assignment on a separate line and looked at the compiled code:

; 118  :         _socket = socket(AF_INET, SOCK_DGRAM, 0);

  00131 8b f4        mov     esi, esp
  00133 6a 00        push    0
  00135 6a 02        push    2
  00137 6a 02        push    2
  00139 ff 15 00 00 00
    00       call    DWORD PTR __imp__socket@12
  0013f 3b f4        cmp     esi, esp
  00141 e8 00 00 00 00   call    __RTC_CheckEsp
  00146 8b 4d f4     mov     ecx, DWORD PTR _this$[ebp]
  00149 89 41 78     mov     DWORD PTR [ecx+120], eax

; 119  :          if (_socket == -1) {

  0014c 8b 45 f4     mov     eax, DWORD PTR _this$[ebp]
  0014f 83 78 78 ff  cmp     DWORD PTR [eax+120], -1
  00153 75 02        jne     SHORT $LN1@open

; 120  :             // loglog("RemoteSyslogAppender: failed to open socket");
; 121  :             return; // fail silently                    

  00155 eb 11        jmp     SHORT $LN9@open
$LN1@open:

; 122  :         }

*Note: I hadd to add a dummy statement after the if statement as the compiler wanted to optimize the code and removed it all - in the current form whether or not the result is an error is irrelevant because in both cases the function just returns and nothing else happens.
*

Thanks,
Vladimir

[Alexander Perepelkin]

Vladimir,

I realized that you were talking about WIN32 platform, where socket is of type SOCKET which is in fact unsigned (in contrast to POSIX)
typedef UINT_PTR SOCKET;
https://msdn.microsoft.com/en-us/library/windows/desktop/ms740506(v=vs.85).aspx
I'll make that check platform-dependent.
Thank you for noticing that.

Alexander

[vstrinski]

Yeah, Microsoft did get us a little surprise with that :)
Thanks Alexander!