system/5395: authpf: change_filter() error handling + cleanup

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

system/5395: authpf: change_filter() error handling + cleanup

Stefan Krah
>Number:         5395
>Category:       system
>Synopsis:       authpf: change_filter() error handling + cleanup
>Confidential:   yes
>Severity:       non-critical
>Priority:       low
>Responsible:    bugs
>State:          open
>Quarter:        
>Keywords:      
>Date-Required:
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sat Feb 24 13:50:02 GMT 2007
>Closed-Date:
>Last-Modified:
>Originator:     Stefan Krah
>Release:        OpenBSD 4.0
>Organization:
net
>Environment:
        <machine, os, target, libraries (multiple lines)>
        System      : OpenBSD 4.0
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:

1)

  - If fork() fails in change_filter(), authpf just exits. It would be possible
    to return -1, then the rest of the cleanup (remove pidfile) could still happen.

  - Why test for WIFEXITED after it has been established that pfctl did not
    complete successfully? A successful test for WIFSIGNALED would also
    indicate an error.


2)

  - unused parameter in change_table()

  - local variables are reset to NULL just before returning in change_filter()


>How-To-Repeat:
>Fix:


Index: authpf.c
===================================================================
RCS file: /cvs/src/usr.sbin/authpf/authpf.c,v
retrieving revision 1.101
diff -u -r1.101 authpf.c
--- authpf.c 22 Feb 2007 21:54:23 -0000 1.101
+++ authpf.c 24 Feb 2007 12:49:58 -0000
@@ -56,7 +56,7 @@
 static int check_luser(char *, char *);
 static int remove_stale_rulesets(void);
 static int change_filter(int, const char *, const char *);
-static int change_table(int, const char *, const char *);
+static int change_table(int, const char *);
 static void authpf_kill_states(void);
 
 int dev; /* pf device */
@@ -297,7 +297,7 @@
  printf("Unable to modify filters\r\n");
  do_death(0);
  }
- if (change_table(1, luser, ipsrc) == -1) {
+ if (change_table(1, ipsrc) == -1) {
  printf("Unable to modify table\r\n");
  change_filter(0, luser, ipsrc);
  do_death(0);
@@ -684,7 +684,8 @@
 
  switch (pid = fork()) {
  case -1:
- err(1, "fork failed");
+ syslog(LOG_ERR, "fork failed");
+ goto error;
  case 0:
  /* revoke group privs before exec */
  gid = getgid();
@@ -699,10 +700,8 @@
  /* parent */
  waitpid(pid, &s, 0);
  if (s != 0) {
- if (WIFEXITED(s)) {
- syslog(LOG_ERR, "pfctl exited abnormally");
- goto error;
- }
+ syslog(LOG_ERR, "pfctl exited abnormally");
+ goto error;
  }
 
  if (add) {
@@ -718,15 +717,10 @@
  syslog(LOG_ERR, "malloc failed");
 error:
  free(fdpath);
- fdpath = NULL;
  free(rsn);
- rsn = NULL;
  free(userstr);
- userstr = NULL;
  free(ipstr);
- ipstr = NULL;
  free(fn);
- fn = NULL;
  return (-1);
 }
 
@@ -734,7 +728,7 @@
  * Add/remove this IP from the "authpf_users" table.
  */
 static int
-change_table(int add, const char *luser, const char *ipsrc)
+change_table(int add, const char *ipsrc)
 {
  struct pfioc_table io;
  struct pfr_addr addr;
@@ -830,7 +824,7 @@
 
  if (active) {
  change_filter(0, luser, ipsrc);
- change_table(0, luser, ipsrc);
+ change_table(0, ipsrc);
  authpf_kill_states();
  remove_stale_rulesets();
  }


>Release-Note:
>Audit-Trail:
>Unformatted: