133701 - Bugs found use anti-c lint engine....

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[John Carter]

This is a list of bugs found by antic (http://artho.com/jlint/)

The vast majority of false positives have been removed leaving only
definite bugs and highly suspicious cases.

======================================================================
If USE_REENTRANT_LIBC is defined, fprint of error will occur whether
error or not. Solution: Put braces round it.

mozilla/nsprpub/config/nsinstall.c:424:9: May be wrong assumption about IF body

    if (error)

#ifdef USE_REENTRANT_LIBC
    R_STRERROR_R(errno);
	fprintf(stderr, ": %s", r_strerror_r);
#else
	fprintf(stderr, ": %s", strerror(errno));
#endif

======================================================================
Verbose mode will also set debug mode on. Is that what was meant?

mozilla/nsprpub/pr/src/cplus/tests/switch.cpp:143:9: Possible miss of BREAK
before CASE/DEFAULT
        switch (opt->option)
        {
        case 'v':  /* verbose mode */
			verbosity = PR_TRUE;
        case 'd':  /* debug mode */
			debug_mode = PR_TRUE;
            break;
======================================================================

I'm not sure if this is a bug, but it looks like it on the
surface. Looks like the first two cases are missing a "break;"

mozilla/nsprpub/pr/tests/nblayer.c:437:9: Possible miss of BREAK before CASE/DEFAULT


        switch (mine->rcvstate)
        {
        case rcv_get_debit:
            b = (char*)&mine->rcvreq;
            mine->rcvreq = amount;
            rv = lo->methods->recv(
                lo, b + mine->rcvinprogress,
                sizeof(mine->rcvreq) - mine->rcvinprogress, flags, timeout);
            if (0 == rv) goto closed;
            if ((-1 == rv) && (PR_WOULD_BLOCK_ERROR == PR_GetError())) break;
            mine->rcvinprogress += rv;  /* accumulate the read */
            if (mine->rcvinprogress < sizeof(mine->rcvreq)) break;  /* loop */
            mine->rcvstate = rcv_send_credit;
            mine->rcvinprogress = 0;
        case rcv_send_credit:
            b = (char*)&mine->rcvreq;
            rv = lo->methods->send(
                lo, b + mine->rcvinprogress,
                sizeof(mine->rcvreq) - mine->rcvinprogress, flags, timeout);
            if ((-1 == rv) && (PR_WOULD_BLOCK_ERROR == PR_GetError())) break;
            mine->rcvinprogress += rv;  /* accumulate the read */
            if (mine->rcvinprogress < sizeof(mine->rcvreq)) break;  /* loop */
            mine->rcvstate = rcv_data;
            mine->rcvinprogress = 0;
        case rcv_data:
            b = (char*)buf;
            rv = lo->methods->recv(
                lo, b + mine->rcvinprogress,
                mine->rcvreq - mine->rcvinprogress, flags, timeout);
            if (0 == rv) goto closed;
            if ((-1 == rv) && (PR_WOULD_BLOCK_ERROR == PR_GetError())) break;
            mine->rcvinprogress += rv;  /* accumulate the read */
            if (mine->rcvinprogress < amount) break;  /* loop */
            mine->rcvstate = rcv_get_debit;
            mine->rcvinprogress = 0;
            return mine->rcvreq;  /* << -- that's it! */

Comment 1

[John Carter]

Whoopsy, copy/paste missed some of it...

            return mine->rcvreq;  /* << -- that's it! */
        default:
            break;
        }
    } while (-1 != rv);

======================================================================

And here it is again on the xmit side... Perhaps it isn't a bug....
mozilla/nsprpub/pr/tests/nblayer.c:494:9: Possible miss of BREAK before CASE/DEFAULT
        switch (mine->xmtstate)
        {
        case xmt_send_debit:
            b = (char*)&mine->xmtreq;
            mine->xmtreq = amount;
            rv = lo->methods->send(
                lo, b - mine->xmtinprogress,
                sizeof(mine->xmtreq) - mine->xmtinprogress, flags, timeout);
            if ((-1 == rv) && (PR_WOULD_BLOCK_ERROR == PR_GetError())) break;
            mine->xmtinprogress += rv;
            if (mine->xmtinprogress < sizeof(mine->xmtreq)) break;
            mine->xmtstate = xmt_recv_credit;
            mine->xmtinprogress = 0;
        case xmt_recv_credit:
             b = (char*)&mine->xmtreq;
             rv = lo->methods->recv(
                lo, b + mine->xmtinprogress,
                sizeof(mine->xmtreq) - mine->xmtinprogress, flags, timeout);
            if ((-1 == rv) && (PR_WOULD_BLOCK_ERROR == PR_GetError())) break;
            mine->xmtinprogress += rv;
            if (mine->xmtinprogress < sizeof(mine->xmtreq)) break;
            mine->xmtstate = xmt_data;
            mine->xmtinprogress = 0;
======================================================================
Definite mismatch of "if" and "elses"...
mozilla/nsprpub/pr/tests/ntioto.c:138:5: May be wrong assumption about ELSE
branch association

    if ( bytesRead == -1 && PR_GetError() == PR_IO_TIMEOUT_ERROR )
      if ( debug ) printf("AcceptRead timed out\n");
    else
        if ( debug ) printf("Oops! read: %d, error: %d\n", bytesRead,
PR_GetError());
======================================================================
We had this one before in switch.cpp above....
mozilla/nsprpub/pr/tests/pollable.c:152:13: Possible miss of BREAK before
CASE/DEFAULT
        switch (opt->option) {
            case 'v':  /* verbose mode */
                verbosity = PR_TRUE;
            case 'd':  /* debug mode */
                debug_mode = PR_TRUE;
                break;
======================================================================
Had this one in ntioto.c above as well....

mozilla/nsprpub/pr/tests/server_test.c:262:9: May be wrong assumption about ELSE
branch association

        if (bytesWritten != _server_data)
          if (debug_mode) printf("\tError sending data to client (%d, %d)\n", 
                                 bytesWritten, PR_GetOSError());
          else
            if (debug_mode) DPRINTF("\tServer sent %d bytes\n", bytesWritten);

======================================================================
And again... (Do I smell copy&paste)....
mozilla/nsprpub/pr/tests/servr_kk.c:228:9: May be wrong assumption about ELSE
branch association

        if (bytesWritten != _server_data)
          if (debug_mode) printf("\tError sending data to client (%d, %d)\n", 
                                 bytesWritten, PR_GetOSError());
          else
            if (debug_mode) DPRINTF("\tServer sent %d bytes\n", bytesWritten);
======================================================================
And again!
mozilla/nsprpub/pr/tests/servr_ku.c:229:9: May be wrong assumption about ELSE
branch association

        if (bytesWritten != _server_data)
            if (debug_mode) printf("\tError sending data to client (%d, %d)\n", 
                bytesWritten, PR_GetOSError());
        else
            if (debug_mode) DPRINTF("\tServer sent %d bytes\n", bytesWritten);
======================================================================
And again....
mozilla/nsprpub/pr/tests/servr_uk.c:231:9: May be wrong assumption about ELSE
branch association
        if (bytesWritten != _server_data)
          if (debug_mode) printf("\tError sending data to client (%d, %d)\n", 
                                 bytesWritten, PR_GetOSError());
          else
            if (debug_mode) DPRINTF("\tServer sent %d bytes\n", bytesWritten);
======================================================================
Ho hum, I recognize this!
mozilla/nsprpub/pr/tests/servr_uu.c:229:9: May be wrong assumption about ELSE
branch association

          if (debug_mode) printf("\tError sending data to client (%d, %d)\n", 
                                 bytesWritten, PR_GetOSError());
          else
            if (debug_mode) DPRINTF("\tServer sent %d bytes\n", bytesWritten);
======================================================================
At least the behaviour is consistent ;-)
mozilla/nsprpub/pr/tests/switch.c:130:9: Possible miss of BREAK before CASE/DEFAULT

        switch (opt->option)
        {
        case 'v':  /* verbose mode */
			verbosity = PR_TRUE;
        case 'd':  /* debug mode */
			debug_mode = PR_TRUE;
            break;

Comment 2

[Tim]

Attachment: Patch to correct most of the problems

This makes most of the changes suggested above.  I'm too uncomfortable to patch
the larger switch block, because I can't tell as easily if that's expected or
not.

/me would think much nicer of lint authors if they would make their programs
produce universal-diff patches containing the suggested changes

Comment 3

[Wan-Teh Chang]

Comment on attachment 92123 [details] [diff] [review]
Patch to correct most of the problems

Thank you for the patch.

The changes that look like this are incorrect:

>         {
>         case 'v':  /* verbose mode */
> 			verbosity = PR_TRUE;
>+            break;
>         case 'd':  /* debug mode */
> 			debug_mode = PR_TRUE;
>             break;

The original code is correct.  If you really want to
eliminate the warnings, the code under case 'd' should
be duplicated for case 'v', like this:

>         {
>         case 'v':  /* verbose mode */
> 			verbosity = PR_TRUE;
>+ 			debug_mode = PR_TRUE;
>+            break;
>         case 'd':  /* debug mode */
> 			debug_mode = PR_TRUE;
>             break;

The changes to the nested if-else statements are good.

Comment 4

[Tim]

Attachment: Address wtc's comments

This is just my previous patch with the changes WTC suggested

Comment 5

[BugBot [:suhaib / :marco]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:KaiE, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 6

[Kai Engert [:KaiE:]]

It seems someone had already applied these fixes.
The only missing one is the verbosity in pollable.c - not worth it.