Bug 71631

Summary: [patch] cleanup of the usr.sbin/pppctl code
Product: Base System Reporter: Dan Lukes <dan>
Component: binAssignee: freebsd-bugs (Nobody) <bugs>
Status: Open ---    
Severity: Affects Some People CC: ngie
Priority: Normal Keywords: patch
Version: 5.3-BETA3   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
file.diff none

Description Dan Lukes 2004-09-12 03:40:30 UTC
	There are more than 5000 warnings issued during "make buildworld".
Some of them are false positives, but some of them are sign of true errors.

	Nobody is upset by warnings due it's amount, so some errors remain
uncorrected.

	I want to cleanup the code-base from warnings, so warnings will
become "attention mark" again.

usr.sbin/pppctl/pppctl.c:368: warning: 'fd' might be used uninitialized in this function
'fd' is initialized with no exception, so turn-off warning only

How-To-Repeat: 	N/A
Comment 1 Giorgos Keramidas freebsd_committer freebsd_triage 2004-09-12 20:50:50 UTC
On 2004-09-12 04:38, Dan Lukes <dan@obluda.cz> wrote:
> usr.sbin/pppctl/pppctl.c:368: warning: 'fd' might be used uninitialized in this function
> 'fd' is initialized with no exception, so turn-off warning only

It can't really.  But this is not a good fix for the warning:

> -     int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
> +     int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
> +     int fd = fd;	/* init to avoid "might be used unitialized" warning 8/

fd = -1; would be a better initialization, since no valid descriptor can
ever be negative and this will expose any bugs that using fd before a
proper initialization can trigger.

The following patch fixes a few other warnings too, and allows me to build
pppctl with WARNS?=6 in today's 6.0-CURRENT:

%%%
Index: pppctl.c
===================================================================
RCS file: /home/ncvs/src/usr.sbin/pppctl/pppctl.c,v
retrieving revision 1.32
diff -u -r1.32 pppctl.c
--- pppctl.c	7 Dec 2003 08:39:29 -0000	1.32
+++ pppctl.c	12 Sep 2004 19:47:32 -0000
@@ -77,7 +77,7 @@
  * How to use pppctl...
  */
 static int
-usage()
+usage(void)
 {
     fprintf(stderr, "usage: pppctl [-v] [-t n] [-p passwd] "
             "Port|LocalSock [command[;command]...]\n");
@@ -93,7 +93,7 @@
  * Handle the SIGALRM received due to a connect() timeout.
  */
 static void
-Timeout(int Sig)
+Timeout(int Sig __unused)
 {
     TimedOut = 1;
 }
@@ -103,10 +103,12 @@
  * All the work is done in Receive() below.
  */
 static char *
-GetPrompt(EditLine *e)
+GetPrompt(EditLine *e __unused)
 {
+    static char empty_prompt[] = "";
+
     if (prompt == NULL)
-        prompt = "";
+        prompt = empty_prompt;
     return prompt;
 }
 
@@ -119,6 +121,7 @@
 Receive(int fd, int display)
 {
     static char Buffer[LINELEN];
+    static char empty_prompt[] = "";
     struct timeval t;
     int Result;
     char *last;
@@ -173,7 +176,7 @@
                 Result = 0;
             break;
         } else
-            prompt = "";
+            prompt = empty_prompt;
         if (len == sizeof Buffer - 1) {
             int flush;
             if ((last = strrchr(Buffer, '\n')) == NULL)
@@ -202,7 +205,7 @@
  * Note, this is a signal handler - be careful of what we do !
  */
 static void
-InputHandler(int sig)
+InputHandler(int sig __unused)
 {
     static char buf[LINELEN];
     struct timeval t;
@@ -365,9 +368,11 @@
 main(int argc, char **argv)
 {
     struct sockaddr_un ifsun;
-    int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
+    int fd = -1;
+    int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
     unsigned TimeoutVal;
-    char *DoneWord = "x", *next, *start;
+    char DoneWord[] = "x";
+    char *next, *start;
     struct sigaction act, oact;
     void *thread_ret;
     pthread_t mon;
@@ -431,7 +436,7 @@
                             hide1off, argv[harg]);
           else
             n = 0;
-          if (n < 0 || n >= sizeof title - pos)
+          if (n < 0 || (size_t)n >= sizeof title - pos)
             break;
           pos += n;
         }
@@ -553,7 +558,7 @@
     len = 0;
     Command[sizeof(Command)-1] = '\0';
     for (arg++; arg < argc; arg++) {
-        if (len && len < sizeof(Command)-1)
+        if (len && (size_t) len < sizeof(Command)-1)
             strcpy(Command+len++, " ");
         strncpy(Command+len, argv[arg], sizeof(Command)-len-1);
         len += strlen(Command+len);
%%%
Comment 2 Dan Lukes 2004-09-13 02:11:47 UTC
On Sun, 12 Sep 2004, Giorgos Keramidas wrote:

>> -     int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
>> +     int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
>> +     int fd = fd;	/* init to avoid "might be used unitialized" warning 8/
>
> fd = -1; would be a better initialization, since no valid descriptor can
> ever be negative and this will expose any bugs that using fd before a
> proper initialization can trigger.

 	But unnecesarry over-initialisation is waste of resources.

 	It's about decision ...

 	I have no opinion which is better (generally).
Comment 3 Giorgos Keramidas freebsd_committer freebsd_triage 2004-09-13 06:56:42 UTC
On 2004-09-13 03:11, Dan Lukes <dan@obluda.cz> wrote:
> On Sun, 12 Sep 2004, Giorgos Keramidas wrote:
> >>-     int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
> >>+     int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
> >>+     int fd = fd;	/* init to avoid "might be used unitialized" warning 8/
> >
> >fd = -1; would be a better initialization, since no valid descriptor can
> >ever be negative and this will expose any bugs that using fd before a
> >proper initialization can trigger.
>
> 	But unnecesarry over-initialisation is waste of resources.
> 	It's about decision ...

Setting `int fd = fd' is also an initialiation that is a waste of
resources.  I see no reason why it's better than an initialization
that also buys us some safety.
Comment 4 Dan Lukes 2004-09-13 07:37:55 UTC
> Setting `int fd = fd' is also an initialiation that is a waste of
> resources.  

	Not true as it's optimized-out despite of O level. It's
hack/information for compiler, not real assignment statement.

> I see no reason why it's better than an initialization
> that also buys us some safety.

	Agree. Lets initialise fd to -1

Dan
Comment 5 Enji Cooper freebsd_committer freebsd_triage 2015-11-10 11:13:54 UTC
This patch isn't correct. I'll take this PR and fix the warnings.
Comment 6 Enji Cooper freebsd_committer freebsd_triage 2017-11-05 21:04:29 UTC
I have no way to test this. Untaking.
Comment 7 Graham Perrin freebsd_committer freebsd_triage 2022-10-17 12:39:11 UTC
Keyword: 

    patch
or  patch-ready

– in lieu of summary line prefix: 

    [patch]

* bulk change for the keyword
* summary lines may be edited manually (not in bulk). 

Keyword descriptions and search interface: 

    <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>