code check

Discussions about the WinBoard protocol. Here you can also report bugs and request new features.

Moderators: hgm, Andres Valverde

code check

Postby F. Bluemers » 19 Feb 2011, 22:47

I stumbled into cppcheck today and gave it a run on the xboard/winboard source code.
Nothing alarming but some things might need to be checked/fixed.

[./winboard/bitmaps/convert.c:73]: (error) Resource leak: f
oh well...,Lets forget this one :D
[./winboard/winboard.c:351]: (error) Resource leak: f
A(nother) file pointer that isn't closed.
Code: Select all
   if((f = fopen(buf, "r")) == NULL) return;
    while((k = fgetc(f)) != EOF) {
        if(i >= sizeof(languageBuf)) { DisplayError("Language file too big", 0); return; }

[./winboard/winboard.c:430]: (error) Uninitialized variable: buf
[./winboard/winboard.c:432]: (error) Uninitialized variable: buf

This one looks like a false alert.buf being updating through the info pointer.
Code: Select all
          char buf[MSG_SIZ];
            info.dwTypeData = buf;
            info.cch = sizeof(buf);
            GetMenuItemInfo(subMenu, j, TRUE, &info);
            if(i < 10) {
                if(menuText[i][j]) safeStrCpy(buf, menuText[i][j], sizeof(buf)/sizeof(buf[0]) );
                else menuText[i][j] = strdup(buf); // remember original on first change
            }

[./winboard/woptions.c:1746]: (error) snprintf size is out of bounds
This one might be more interesting
Code: Select all
void
InitSoundCombo(HWND hwndCombo, SoundComboData *scd)
{
  char buf[255];
  DWORD err;
  DWORD cnt = 0;
  SendMessage(hwndCombo, CB_RESETCONTENT, 0, 0);

  /* send the labels to the combo box */
  while (scd->label) {
    err = SendMessage(hwndCombo, CB_ADDSTRING, 0, (LPARAM) T_(scd->label));
    if (err != cnt++) {
      snprintf(buf, MSG_SIZ,  "InitSoundCombo(): err '%d', cnt '%d'\n",
     (int)err, (int)cnt);
      MessageBox(NULL, buf, NULL, MB_OK);

[xboard.c:3332]: (error) Resource leak: fp
file pointer not closed
[xboard.c:5957]: (error) Resource leak: f
Another fp
[xboard.c:8110]: (error) Memory leak: hp
Code: Select all
int OpenTCP(host, port, pr)
     char *host;
     char *port;
     ProcRef *pr;
{
#if OMIT_SOCKETS
    DisplayFatalError(_("Socket support is not configured in"), 0, 2);
#else  /* !OMIT_SOCKETS */
    int s;
    struct sockaddr_in sa;
    struct hostent     *hp;
    unsigned short uport;
    ChildProc *cp;

    if ((s = socket(AF_INET, SOCK_STREAM, 6)) < 0) {
   return errno;
    }

    memset((char *) &sa, (int)0, sizeof(struct sockaddr_in));
    sa.sin_family = AF_INET;
    sa.sin_addr.s_addr = INADDR_ANY;
    uport = (unsigned short) 0;
    sa.sin_port = htons(uport);
    if (bind(s, (struct sockaddr *) &sa, sizeof(struct sockaddr_in)) < 0) {
   return errno;
    }

    memset((char *) &sa, (int)0, sizeof(struct sockaddr_in));
    if (!(hp = gethostbyname(host))) {
   int b0, b1, b2, b3;
   if (sscanf(host, "%d.%d.%d.%d", &b0, &b1, &b2, &b3) == 4) {
       hp = (struct hostent *) calloc(1, sizeof(struct hostent));
       hp->h_addrtype = AF_INET;
       hp->h_length = 4;
       hp->h_addr_list = (char **) calloc(2, sizeof(char *));
       hp->h_addr_list[0] = (char *) malloc(4);
       hp->h_addr_list[0][0] = b0;
       hp->h_addr_list[0][1] = b1;
       hp->h_addr_list[0][2] = b2;
       hp->h_addr_list[0][3] = b3;
   } else {
       return ENOENT;
   }
    }
    sa.sin_family = hp->h_addrtype;
    uport = (unsigned short) atoi(port);
    sa.sin_port = htons(uport);
    memcpy((char *) &sa.sin_addr, hp->h_addr, hp->h_length);

    if (connect(s, (struct sockaddr *) &sa,
      sizeof(struct sockaddr_in)) < 0) {
   return errno;
    }

    cp = (ChildProc *) calloc(1, sizeof(ChildProc));
    cp->kind = CPSock;
    cp->pid = 0;
    cp->fdFrom = s;
    cp->fdTo = s;
    *pr = (ProcRef) cp;

#endif /* !OMIT_SOCKETS */

    return 0;
}

[xboard.c:4984]: (error) Uninitialized variable: xx
[xboard.c:4985]: (error) Uninitialized variable: yy

cppcheck goes through all the #ifdefs... :D
Code: Select all
 
#ifdef NOTDEF
    /* This code seems to tickle an X bug if it is executed too soon
       after xboard starts up.  The coordinates get transformed as if
       the main window was positioned at (0, 0).
    */
    XtTranslateCoords(shellWidget, (bw_width - w) / 2, 0 - h / 2, &x, &y);
#else  /*!NOTDEF*/
    XTranslateCoordinates(xDisplay, XtWindow(shellWidget),
           RootWindowOfScreen(XtScreen(shellWidget)),
           (bw_width - w) / 2, 0 - h / 2, &xx, &yy, &junk);
#endif /*!NOTDEF*/
    x = xx;
    y = yy;

Best
Fonzy
F. Bluemers
 
Posts: 175
Joined: 04 Sep 2008, 16:56
Location: Netherlands

Re: code check

Postby H.G.Muller » 19 Feb 2011, 23:46

OK, thanks!

Most of them are not very spectacular. (The use of snprintf for a string that you know will fit is overkill anyway, and some of the resource/memory leaks seem to be on one-time events, like connecting to the ICS, or events that are not supposed to happen, like too-big language files.)

But the xboard.c:5957 file pointer leak could be bad, as this is in the code that moves stuff from or to the clipboard. So it could potentially be used many times. It was added recently (by Tim!). I guess I'd better fix this...
User avatar
H.G.Muller
 
Posts: 3260
Joined: 16 Nov 2005, 12:02
Location: Diemen, NL


Return to WinBoard development and bugfixing

Who is online

Users browsing this forum: No registered users and 5 guests