Re: fix crontab(1) NULL pointer dereference
On Sat, 5 Feb 2011, Lawrence Teo wrote:
> When "crontab -l" is used to list a user's crontab file, crontab(1)
> expects the crontab file to have three comment lines at the top.
>
> However, if there are fewer than three comment lines or if they are
> completely absent, crontab(1) will segfault when the
> ignore_comments() function tries to use putc() to write to the
> NewCrontab FILE pointer, which is NULL since NewCrontab is never
> opened when "crontab -l" is used.
>
> For example:
>
> # cat /var/cron/tabs/lteo
> 0 0 * * * /bin/true
> # crontab -u lteo -l
> Segmentation fault
>
> The following diff fixes this bug by telling ignore_comments() to
> use putchar() if NewCrontab is NULL so that the crontab file is
> written to stdout instead. It also explicitly initializes
> NewCrontab to NULL at the beginning of main().
I think it makes more sense to explicitly pass the function the FILE to
write to instead and to push the rest of the 'copy' logic into the
function instead of having slightly different code after the
ignore_comments() call.
(Yes, the fclose(f) could be pushed into the function, but the lifetime of
the FILE is clearer if it isn't.)
This fixes the reported problem in my testing.
oks?
Philip Guenther
Index: crontab.c
===
RCS file: /cvs/src/usr.sbin/cron/crontab.c,v
retrieving revision 1.59
diff -u -p -r1.59 crontab.c
--- crontab.c 31 Jan 2011 18:02:56 - 1.59
+++ crontab.c 6 Feb 2011 05:41:38 -
@@ -56,9 +56,9 @@ staticvoidlist_cmd(void),
edit_cmd(void),
check_error(const char *),
parse_args(int c, char *v[]),
+ copy_crontab(FILE *, FILE *),
die(int);
static int replace_cmd(void);
-static int ignore_comments(FILE *);
static void
usage(const char *msg) {
@@ -251,12 +251,7 @@ list_cmd(void) {
*/
Set_LineNum(1)
- /* ignore the top few comments since we probably put them there.
-*/
- ch = ignore_comments(f);
-
- while (EOF != (ch = get_char(f)))
- putchar(ch);
+ copy_crontab(f, stdout);
fclose(f);
}
@@ -360,15 +355,7 @@ edit_cmd(void) {
Set_LineNum(1)
- /* ignore the top few comments since we probably put them there.
-*/
- ch = ignore_comments(f);
-
- /* copy the rest of the crontab (if any) to the temp file.
-*/
- if (EOF != ch)
- while (EOF != (ch = get_char(f)))
- putc(ch, NewCrontab);
+ copy_crontab(f, NewCrontab);
fclose(f);
if (fflush(NewCrontab) < OK) {
perror(Filename);
@@ -686,14 +673,16 @@ die(int x) {
_exit(ERROR_EXIT);
}
-static int
-ignore_comments(FILE *f) {
+static void
+copy_crontab(FILE *f, FILE *out) {
int ch, x;
+ /* ignore the top few comments since we probably put them there.
+*/
x = 0;
while (EOF != (ch = get_char(f))) {
if ('#' != ch) {
- putc(ch, NewCrontab);
+ putc(ch, out);
break;
}
while (EOF != (ch = get_char(f)))
@@ -703,5 +692,9 @@ ignore_comments(FILE *f) {
break;
}
- return ch;
+ /* copy out the rest of the crontab (if any)
+*/
+ if (EOF != ch)
+ while (EOF != (ch = get_char(f)))
+ putc(ch, out);
}
How To Build Muscle and Gain Weight Fast
who can't build muscle or gain weight no matter how hard they try. Take just five minutes to read this entire page and you'll discover. Start Now : http://good-links.us/how-to-build-muscle-and-gain-weight-fast.html
httpd(8) - allow location of etag-state to be configured
This allows the location of the etag-state file to be configured in
httpd.conf rather than hardcoded to "logs/etag-state".
Index: conf/httpd.conf
===
RCS file: /home/danh/.cvs/openbsd/src/usr.sbin/httpd/conf/httpd.conf,v
retrieving revision 1.26
diff -u -p -r1.26 httpd.conf
--- conf/httpd.conf 3 Jun 2009 18:28:21 - 1.26
+++ conf/httpd.conf 5 Feb 2011 08:06:09 -
@@ -85,6 +85,10 @@ ServerRoot "/var/www"
#
PidFile logs/httpd.pid
#
+# ETagFile: File used to store the ETag initialization secret.
+#
+ETagFile logs/etag-state
+#
# ScoreBoardFile: File used to store internal server process information.
# Not all architectures require this. But if yours does (you'll know because
# this file will be created when you run Apache) then you *must* ensure that
Index: src/include/http_conf_globals.h
===
RCS file:
/home/danh/.cvs/openbsd/src/usr.sbin/httpd/src/include/http_conf_globals.h,v
retrieving revision 1.17
diff -u -p -r1.17 http_conf_globals.h
--- src/include/http_conf_globals.h 9 May 2008 08:06:28 - 1.17
+++ src/include/http_conf_globals.h 5 Feb 2011 07:46:15 -
@@ -97,6 +97,7 @@ extern API_VAR_EXPORT int ap_extended_st
extern API_VAR_EXPORT ap_ctx *ap_global_ctx;
extern API_VAR_EXPORT char *ap_pid_fname;
+extern API_VAR_EXPORT char *ap_etag_fname;
extern API_VAR_EXPORT char *ap_scoreboard_fname;
extern API_VAR_EXPORT char *ap_lock_fname;
extern API_VAR_EXPORT char *ap_server_argv0;
Index: src/include/httpd.h
===
RCS file: /home/danh/.cvs/openbsd/src/usr.sbin/httpd/src/include/httpd.h,v
retrieving revision 1.30
diff -u -p -r1.30 httpd.h
--- src/include/httpd.h 25 Feb 2010 07:49:53 - 1.30
+++ src/include/httpd.h 5 Feb 2011 07:44:31 -
@@ -165,6 +165,9 @@ extern "C" {
#ifndef DEFAULT_PIDLOG
#define DEFAULT_PIDLOG "logs/httpd.pid"
#endif
+#ifndef DEFAULT_ETAGLOG
+#define DEFAULT_ETAGLOG "logs/etag-state"
+#endif
#ifndef DEFAULT_SCOREBOARD
#define DEFAULT_SCOREBOARD "logs/apache_runtime_status"
#endif
Index: src/main/http_config.c
===
RCS file: /home/danh/.cvs/openbsd/src/usr.sbin/httpd/src/main/http_config.c,v
retrieving revision 1.21
diff -u -p -r1.21 http_config.c
--- src/main/http_config.c 14 May 2008 16:11:22 - 1.21
+++ src/main/http_config.c 5 Feb 2011 07:45:09 -
@@ -1604,6 +1604,7 @@ init_config_globals(pool *p)
ap_daemons_max_free = DEFAULT_MAX_FREE_DAEMON;
ap_daemons_limit = HARD_SERVER_LIMIT;
ap_pid_fname = DEFAULT_PIDLOG;
+ ap_etag_fname = DEFAULT_ETAGLOG;
ap_scoreboard_fname = DEFAULT_SCOREBOARD;
ap_lock_fname = DEFAULT_LOCKFILE;
ap_max_requests_per_child = DEFAULT_MAX_REQUESTS_PER_CHILD;
Index: src/main/http_core.c
===
RCS file: /home/danh/.cvs/openbsd/src/usr.sbin/httpd/src/main/http_core.c,v
retrieving revision 1.27
diff -u -p -r1.27 http_core.c
--- src/main/http_core.c10 May 2010 02:00:50 - 1.27
+++ src/main/http_core.c5 Feb 2011 08:07:04 -
@@ -2019,6 +2019,20 @@ static const char *set_pidfile(cmd_parms
return NULL;
}
+static const char *set_etagfile(cmd_parms *cmd, void *dummy, char *arg)
+{
+const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
+if (err != NULL) {
+return err;
+}
+
+if (cmd->server->is_virtual) {
+ return "ETagFile directive not allowed in ";
+}
+ap_etag_fname = arg;
+return NULL;
+}
+
static const char *set_scoreboard(cmd_parms *cmd, void *dummy, char *arg)
{
const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
@@ -3164,6 +3178,8 @@ static const command_rec core_cmds[] = {
"The filename of the error log" },
{ "PidFile", set_pidfile, NULL, RSRC_CONF, TAKE1,
"A file for logging the server process ID"},
+{ "ETagFile", set_etagfile, NULL, RSRC_CONF, TAKE1,
+"A file for storing the ETag initialization secret"},
{ "ScoreBoardFile", set_scoreboard, NULL, RSRC_CONF, TAKE1,
"A file for Apache to maintain runtime process management information"},
{ "LockFile", set_lockfile, NULL, RSRC_CONF, TAKE1,
Index: src/main/http_main.c
===
RCS file: /home/danh/.cvs/openbsd/src/usr.sbin/httpd/src/main/http_main.c,v
retrieving revision 1.54
diff -u -p -r1.54 http_main.c
--- src/main/http_main.c25 Feb 2010 07:49:53 - 1.54
+++ src/main/http_main.c5 Feb 2011 07:54:49 -
@@ -165,6 +165,7 @@ API_VAR_EXPORT int ap_max_stack_per_chil
API_VAR_EXPORT int ap_threads_per_child=0;
API_VAR_EXPORT int ap_excess_requests_per_child=0;
API_VAR_EXPORT char *ap_pid_fname=NULL;
+API_VAR_EXPORT char *ap_etag_fname=NULL;
API_VAR_EXPORT char *ap_scorebo
Re: incorrect fallthrough in pf
On Sat, Feb 05, 2011 at 07:51:27PM +0100, Henning Brauer wrote:
> indeed. and as much as i'm all for defensive programming, pf_test_rule
> will never be called from anything but pf_test[6] - at least without
> heavy heavy major super duper changes, besides there not being a reson
> to. thus:
I checked the call graph again and found this:
pf_test() -> pflog_packet() -> bpf_mtap_pflog() -> bpf_catchpacket()
-> pflog_bpfcopy() -> pf_translate()
It is not obvious anymore that IPv4-ICMP6 and IPv6-ICMP cannot occur
in pf_translate(). I recommend defensive programming in that case.
So I'd like to commit a combination of my previous two diffs.
ok?
bluhm
pf_test() and pf_test6() drop IPv4-ICMP6 and IPv6-ICMP packets. Do
not do the same check in pf_test_rule() again.
Index: net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.723
diff -u -p -r1.723 pf.c
--- net/pf.c5 Feb 2011 17:29:05 - 1.723
+++ net/pf.c5 Feb 2011 17:50:32 -
@@ -2776,8 +2776,6 @@ pf_test_rule(struct pf_rule **rm, struct
break;
#ifdef INET
case IPPROTO_ICMP:
- if (pd->af != AF_INET)
- break;
icmptype = pd->hdr.icmp->icmp_type;
icmpcode = pd->hdr.icmp->icmp_code;
state_icmp = pf_icmp_mapping(pd, icmptype,
@@ -2793,8 +2791,6 @@ pf_test_rule(struct pf_rule **rm, struct
#endif /* INET */
#ifdef INET6
case IPPROTO_ICMPV6:
- if (af != AF_INET6)
- break;
icmptype = pd->hdr.icmp6->icmp6_type;
icmpcode = pd->hdr.icmp6->icmp6_code;
state_icmp = pf_icmp_mapping(pd, icmptype,
pf_translate() may be called from pflog_packet(). Make sure that
IPv4-ICMP6 and IPv6-ICMP packets are handled correctly in case they
are dropped and logged.
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.722
diff -u -p -r1.722 pf.c
--- net/pf.c22 Jan 2011 11:43:57 - 1.722
+++ net/pf.c5 Feb 2011 13:43:58 -
@@ -3281,8 +3281,7 @@ pf_translate(struct pf_pdesc *pd, struct
if (PF_ANEQ(daddr, pd->dst, pd->af))
pd->destchg = 1;
- switch (pd->proto) {
- case IPPROTO_TCP:
+ if (pd->proto == IPPROTO_TCP) {
if (PF_ANEQ(saddr, pd->src, pd->af) || *pd->sport != sport) {
pf_change_ap(pd->src, pd->sport, pd->ip_sum,
&pd->hdr.tcp->th_sum, saddr, sport, 0, pd->af);
@@ -3293,9 +3292,7 @@ pf_translate(struct pf_pdesc *pd, struct
&pd->hdr.tcp->th_sum, daddr, dport, 0, pd->af);
rewrite = 1;
}
- break;
-
- case IPPROTO_UDP:
+ } else if (pd->proto == IPPROTO_UDP) {
if (PF_ANEQ(saddr, pd->src, pd->af) || *pd->sport != sport) {
pf_change_ap(pd->src, pd->sport, pd->ip_sum,
&pd->hdr.udp->uh_sum, saddr, sport, 1, pd->af);
@@ -3306,10 +3303,8 @@ pf_translate(struct pf_pdesc *pd, struct
&pd->hdr.udp->uh_sum, daddr, dport, 1, pd->af);
rewrite = 1;
}
- break;
-
#ifdef INET
- case IPPROTO_ICMP:
+ } else if (pd->proto == IPPROTO_ICMP && pd->af == AF_INET) {
if (PF_ANEQ(saddr, pd->src, pd->af)) {
pf_change_a(&pd->src->v4.s_addr, pd->ip_sum,
saddr->v4.s_addr, 0);
@@ -3331,28 +3326,21 @@ pf_translate(struct pf_pdesc *pd, struct
rewrite = 1;
}
}
- break;
#endif /* INET */
-
#ifdef INET6
- case IPPROTO_ICMPV6:
- if (pd->af == AF_INET6) {
- if (PF_ANEQ(saddr, pd->src, pd->af)) {
- pf_change_a6(pd->src,
- &pd->hdr.icmp6->icmp6_cksum, saddr, 0);
- rewrite = 1;
- }
- if (PF_ANEQ(daddr, pd->dst, pd->af)) {
- pf_change_a6(pd->dst,
- &pd->hdr.icmp6->icmp6_cksum, daddr, 0);
- rewrite = 1;
- }
- break;
+ } else if (pd->proto == IPPROTO_ICMPV6 && pd->af == AF_INET6) {
+ if (PF_ANEQ(saddr, pd->src, pd->af)) {
+ pf_change_a6(pd->src, &pd->hdr.icmp6->icmp6_cksum,
+ saddr, 0);
+ rewrite = 1;
+ }
+ if (PF_ANEQ(daddr, pd->dst, pd->af)) {
+ pf_change_a6(pd->dst, &pd->hdr.icmp6->icmp6_cksum,
+ daddr, 0);
+
Re: ksh completion for [, :, $
On Fri, Feb 4, 2011 at 1:42 PM, Alexander Polakov wrote: > Hi there, > > I sent this diff to bugs@ some time ago but haven't got any replies. > Probably tech@ is a better place for it. > > The problem is known as bz#6006/user. > > The fix is taken from mksh (rev.1.4 for [ and rev.1.184 for others). > It adds quoting character (QCHAR) when [, $, ` are prepended by \ to make > them be interpreted literally (like if they were enclosed in ' '). > It also adds : to the list of escaped characters. > Original commit message: This. PLEASE. -Nick
Re: fix crontab(1) NULL pointer dereference
On Sat, Feb 05, 2011 at 02:48:54PM -0500, Lawrence Teo wrote:
> When "crontab -l" is used to list a user's crontab file, crontab(1)
> expects the crontab file to have three comment lines at the top.
>
> However, if there are fewer than three comment lines or if they are
> completely absent, crontab(1) will segfault when the
> ignore_comments() function tries to use putc() to write to the
> NewCrontab FILE pointer, which is NULL since NewCrontab is never
> opened when "crontab -l" is used.
>
> For example:
>
> # cat /var/cron/tabs/lteo
> 0 0 * * * /bin/true
> # crontab -u lteo -l
> Segmentation fault
>
> The following diff fixes this bug by telling ignore_comments() to
> use putchar() if NewCrontab is NULL so that the crontab file is
> written to stdout instead. It also explicitly initializes
> NewCrontab to NULL at the beginning of main().
Oops, forgot the diff. :)
Lawrence
Index: crontab.c
===
RCS file: /cvs/src/usr.sbin/cron/crontab.c,v
retrieving revision 1.59
diff -u -p -r1.59 crontab.c
--- crontab.c 31 Jan 2011 18:02:56 - 1.59
+++ crontab.c 5 Feb 2011 19:36:05 -
@@ -79,6 +79,7 @@ main(int argc, char *argv[]) {
Pid = getpid();
ProgramName = argv[0];
+ NewCrontab = NULL;
setlocale(LC_ALL, "");
@@ -693,7 +694,10 @@ ignore_comments(FILE *f) {
x = 0;
while (EOF != (ch = get_char(f))) {
if ('#' != ch) {
- putc(ch, NewCrontab);
+ if (NewCrontab != NULL)
+ putc(ch, NewCrontab);
+ else
+ putchar(ch);
break;
}
while (EOF != (ch = get_char(f)))
fix crontab(1) NULL pointer dereference
When "crontab -l" is used to list a user's crontab file, crontab(1) expects the crontab file to have three comment lines at the top. However, if there are fewer than three comment lines or if they are completely absent, crontab(1) will segfault when the ignore_comments() function tries to use putc() to write to the NewCrontab FILE pointer, which is NULL since NewCrontab is never opened when "crontab -l" is used. For example: # cat /var/cron/tabs/lteo 0 0 * * * /bin/true # crontab -u lteo -l Segmentation fault The following diff fixes this bug by telling ignore_comments() to use putchar() if NewCrontab is NULL so that the crontab file is written to stdout instead. It also explicitly initializes NewCrontab to NULL at the beginning of main(). Lawrence
Re: incorrect fallthrough in pf
* Alexander Bluhm [2011-02-05 19:02]:
> On Sat, Feb 05, 2011 at 03:24:11PM +0100, Henning Brauer wrote:
> > I'm pretty damn sure we catch that way earlier.
>
> You are right.
>
> pf_test():
> case IPPROTO_ICMPV6: {
> action = PF_DROP;
> DPFPRINTF(LOG_NOTICE,
> "dropping IPv4 packet with ICMPv6 payload");
> goto done;
> }
>
> pf_test6():
> case IPPROTO_ICMP: {
> action = PF_DROP;
> DPFPRINTF(LOG_NOTICE,
> "dropping IPv6 packet with ICMPv4 payload");
> goto done;
> }
>
> But then some more checks in pf_test_rule() are dead code and should
> be removed. Either we rely on the checks in pf_test[46]() or we
> don't. But we should do it consistently.
indeed. and as much as i'm all for defensive programming, pf_test_rule
will never be called from anything but pf_test[6] - at least without
heavy heavy major super duper changes, besides there not being a reson
to. thus:
> ok?
yes.
--
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services, http://bsws.de
Full-Service ISP - Secure Hosting, Mail and DNS Services
Dedicated Servers, Rootservers, Application Hosting
Re: incorrect fallthrough in pf
On Sat, Feb 05, 2011 at 03:24:11PM +0100, Henning Brauer wrote:
> I'm pretty damn sure we catch that way earlier.
You are right.
pf_test():
case IPPROTO_ICMPV6: {
action = PF_DROP;
DPFPRINTF(LOG_NOTICE,
"dropping IPv4 packet with ICMPv6 payload");
goto done;
}
pf_test6():
case IPPROTO_ICMP: {
action = PF_DROP;
DPFPRINTF(LOG_NOTICE,
"dropping IPv6 packet with ICMPv4 payload");
goto done;
}
But then some more checks in pf_test_rule() are dead code and should
be removed. Either we rely on the checks in pf_test[46]() or we
don't. But we should do it consistently.
ok?
bluhm
Index: net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.723
diff -u -p -r1.723 pf.c
--- net/pf.c5 Feb 2011 17:29:05 - 1.723
+++ net/pf.c5 Feb 2011 17:50:32 -
@@ -2776,8 +2776,6 @@ pf_test_rule(struct pf_rule **rm, struct
break;
#ifdef INET
case IPPROTO_ICMP:
- if (pd->af != AF_INET)
- break;
icmptype = pd->hdr.icmp->icmp_type;
icmpcode = pd->hdr.icmp->icmp_code;
state_icmp = pf_icmp_mapping(pd, icmptype,
@@ -2793,8 +2791,6 @@ pf_test_rule(struct pf_rule **rm, struct
#endif /* INET */
#ifdef INET6
case IPPROTO_ICMPV6:
- if (af != AF_INET6)
- break;
icmptype = pd->hdr.icmp6->icmp6_type;
icmpcode = pd->hdr.icmp6->icmp6_code;
state_icmp = pf_icmp_mapping(pd, icmptype,
@@ -3336,20 +3332,17 @@ pf_translate(struct pf_pdesc *pd, struct
#ifdef INET6
case IPPROTO_ICMPV6:
- if (pd->af == AF_INET6) {
- if (PF_ANEQ(saddr, pd->src, pd->af)) {
- pf_change_a6(pd->src,
- &pd->hdr.icmp6->icmp6_cksum, saddr, 0);
- rewrite = 1;
- }
- if (PF_ANEQ(daddr, pd->dst, pd->af)) {
- pf_change_a6(pd->dst,
- &pd->hdr.icmp6->icmp6_cksum, daddr, 0);
- rewrite = 1;
- }
- break;
+ if (PF_ANEQ(saddr, pd->src, pd->af)) {
+ pf_change_a6(pd->src, &pd->hdr.icmp6->icmp6_cksum,
+ saddr, 0);
+ rewrite = 1;
+ }
+ if (PF_ANEQ(daddr, pd->dst, pd->af)) {
+ pf_change_a6(pd->dst, &pd->hdr.icmp6->icmp6_cksum,
+ daddr, 0);
+ rewrite = 1;
}
- /* FALLTHROUGH */
+ break;
#endif /* INET6 */
default:
Re: incorrect fallthrough in pf
On Sat, Feb 05, 2011 at 03:24:11PM +0100, Henning Brauer wrote:
> * Alexander Bluhm [2011-02-05 14:56]:
> > Somebody could send us such a packet.
>
> I'm pretty damn sure we catch that way earlier.
Yeah, it panics right away if nat/rdr is used with unusual protocol.
panic: m_clget: request for 1926905556 byte cluster
Stopped at Debugger+0x4: popl%ebp
RUN AT LEAST 'trace' AND 'ps' AND INCLUDE OUTPUT WHEN REPORTING THIS PANIC!
DO NOT EVEN BOTHER REPORTING THIS WITHOUT INCLUDING THAT INFORMATION!
ddb> trace
Debugger(d08dcebc,d45b4bd0,d08bc8c4,d45b4bd0,d0202f0d) at Debugger+0x4
panic(d08bc8c4,72da3ed4,d45b4bf4,d0400453,40) at panic+0x5d
m_clget(d30ea700,2,0,72da3ed4,d30591a8) at m_clget+0x138
m_copyback(d30ea500,14,72da3ed4,d45b4d94,2) at m_copyback+0x1d2
pf_test_rule(d45b4dc0,d45b4dbc,1,d0e04d00,d30ea500) at pf_test_rule+0xd91
pf_test(1,d0f06038,d45b4eac,0,d0f06000) at pf_test+0xcd3
ipv4_input(d30ea500,6,d45b4ec4,d0441185,d0202f0d) at ipv4_input+0x204
ipintr(d0202f0d,d0eefbe0,d45b4ee4,d057ac2f,0) at ipintr+0x49
netintr(0,0,d0ef0a80,0,d0201fc6) at netintr+0xd5
softintr_dispatch(1) at softintr_dispatch+0x4f
Xsoftnet() at Xsoftnet+0x12
hrdlen can be uinitialized. Let's fix that first.
ok?
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.722
diff -u -p -r1.722 pf.c
--- net/pf.c22 Jan 2011 11:43:57 - 1.722
+++ net/pf.c5 Feb 2011 16:16:57 -
@@ -3047,7 +3047,7 @@ pf_test_rule(struct pf_rule **rm, struct
}
/* copy back packet headers if we performed NAT operations */
- if (rewrite)
+ if (rewrite && hdrlen)
m_copyback(m, off, hdrlen, pd->hdr.any, M_NOWAIT);
#if NPFSYNC > 0
@@ -5517,6 +5517,7 @@ pf_setup_pdesc(sa_family_t af, int dir,
if (pd->hdr.any == NULL)
panic("pf_setup_pdesc: no storage for headers provided");
+ *hdrlen = 0;
switch (af) {
#ifdef INET
case AF_INET: {
Re: incorrect fallthrough in pf
On Sat, Feb 5, 2011 at 2:54 PM, Alexander Bluhm wrote: > On Sat, Feb 05, 2011 at 02:03:25PM +0100, Mike Belopuhov wrote: >> we can check if "af == inet" in icmp case obviously, but how and why >> can we end up with af == inet6 and an icmp payload (or af == inet >> and icmp6 payload for that matter)? > > Somebody could send us such a packet. > > It is strange but not illegal. If we decide, pf should drop such > packets that is a complete different thing. > > Anyway we should make sure that pf_translate() can handle these > packets by itself. Protocol 1 for IPv6 packets means no special > treatment so it should end in the else case. > > I suggest that diff (untested): > in this case i'm okay with your diff.
Re: incorrect fallthrough in pf
* Alexander Bluhm [2011-02-05 14:56]: > On Sat, Feb 05, 2011 at 02:03:25PM +0100, Mike Belopuhov wrote: > > we can check if "af == inet" in icmp case obviously, but how and why > > can we end up with af == inet6 and an icmp payload (or af == inet > > and icmp6 payload for that matter)? > > Somebody could send us such a packet. I'm pretty damn sure we catch that way earlier. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services, http://bsws.de Full-Service ISP - Secure Hosting, Mail and DNS Services Dedicated Servers, Rootservers, Application Hosting
Re: incorrect fallthrough in pf
On Sat, Feb 5, 2011 at 1:50 PM, Alexander Bluhm
wrote:
> I think there is a missing check and fallthrough in the icmp case.
>
> The logic should be
>
> if (proto == tcp) {
> } else if (proto == udp) {
> } else if (proto == icmp && af == inet) {
> } else if (proto == icmp6 && af == inet6) {
> } else {
> }
>
> The current code would do icmp processing for an ipv6 packet with
> protocol 1. Such a packet is strange but it should not get special
> translation treatment.
>
> bluhm
>
>
we can check if "af == inet" in icmp case obviously, but how and why
can we end up with af == inet6 and an icmp payload (or af == inet
and icmp6 payload for that matter)?
Re: incorrect fallthrough in pf
On Sat, Feb 05, 2011 at 02:03:25PM +0100, Mike Belopuhov wrote:
> we can check if "af == inet" in icmp case obviously, but how and why
> can we end up with af == inet6 and an icmp payload (or af == inet
> and icmp6 payload for that matter)?
Somebody could send us such a packet.
It is strange but not illegal. If we decide, pf should drop such
packets that is a complete different thing.
Anyway we should make sure that pf_translate() can handle these
packets by itself. Protocol 1 for IPv6 packets means no special
treatment so it should end in the else case.
I suggest that diff (untested):
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.722
diff -u -p -r1.722 pf.c
--- net/pf.c22 Jan 2011 11:43:57 - 1.722
+++ net/pf.c5 Feb 2011 13:43:58 -
@@ -3281,8 +3281,7 @@ pf_translate(struct pf_pdesc *pd, struct
if (PF_ANEQ(daddr, pd->dst, pd->af))
pd->destchg = 1;
- switch (pd->proto) {
- case IPPROTO_TCP:
+ if (pd->proto == IPPROTO_TCP) {
if (PF_ANEQ(saddr, pd->src, pd->af) || *pd->sport != sport) {
pf_change_ap(pd->src, pd->sport, pd->ip_sum,
&pd->hdr.tcp->th_sum, saddr, sport, 0, pd->af);
@@ -3293,9 +3292,7 @@ pf_translate(struct pf_pdesc *pd, struct
&pd->hdr.tcp->th_sum, daddr, dport, 0, pd->af);
rewrite = 1;
}
- break;
-
- case IPPROTO_UDP:
+ } else if (pd->proto == IPPROTO_UDP) {
if (PF_ANEQ(saddr, pd->src, pd->af) || *pd->sport != sport) {
pf_change_ap(pd->src, pd->sport, pd->ip_sum,
&pd->hdr.udp->uh_sum, saddr, sport, 1, pd->af);
@@ -3306,10 +3303,8 @@ pf_translate(struct pf_pdesc *pd, struct
&pd->hdr.udp->uh_sum, daddr, dport, 1, pd->af);
rewrite = 1;
}
- break;
-
#ifdef INET
- case IPPROTO_ICMP:
+ } else if (pd->proto == IPPROTO_ICMP && pd->af == AF_INET) {
if (PF_ANEQ(saddr, pd->src, pd->af)) {
pf_change_a(&pd->src->v4.s_addr, pd->ip_sum,
saddr->v4.s_addr, 0);
@@ -3331,28 +3326,21 @@ pf_translate(struct pf_pdesc *pd, struct
rewrite = 1;
}
}
- break;
#endif /* INET */
-
#ifdef INET6
- case IPPROTO_ICMPV6:
- if (pd->af == AF_INET6) {
- if (PF_ANEQ(saddr, pd->src, pd->af)) {
- pf_change_a6(pd->src,
- &pd->hdr.icmp6->icmp6_cksum, saddr, 0);
- rewrite = 1;
- }
- if (PF_ANEQ(daddr, pd->dst, pd->af)) {
- pf_change_a6(pd->dst,
- &pd->hdr.icmp6->icmp6_cksum, daddr, 0);
- rewrite = 1;
- }
- break;
+ } else if (pd->proto == IPPROTO_ICMPV6 && pd->af == AF_INET6) {
+ if (PF_ANEQ(saddr, pd->src, pd->af)) {
+ pf_change_a6(pd->src, &pd->hdr.icmp6->icmp6_cksum,
+ saddr, 0);
+ rewrite = 1;
+ }
+ if (PF_ANEQ(daddr, pd->dst, pd->af)) {
+ pf_change_a6(pd->dst, &pd->hdr.icmp6->icmp6_cksum,
+ daddr, 0);
+ rewrite = 1;
}
- /* FALLTHROUGH */
#endif /* INET6 */
-
- default:
+ } else {
switch (pd->af) {
#ifdef INET
case AF_INET:
Re: incorrect fallthrough in pf
On Wed, Feb 02, 2011 at 04:14:01PM +0100, Mike Belopuhov wrote:
> hi, in pf_translate, when we're changing addresses for the icmp messages
> there's an unjustified fallthrough in the IPPROTO_ICMPV6 case. in fact
> this doesn't seem to harm anything because default case performs the
> same operation. note that pd->ip_sum is null in ipv6 case so pf_change_a6
> just punches a translation address to the packet with PF_ACPY.
>
> henning@ agrees that this fallthrough was introduced by mistake, but we
> won't mind if somebody with pf knowledge will glance through the code.
I think there is a missing check and fallthrough in the icmp case.
The logic should be
if (proto == tcp) {
} else if (proto == udp) {
} else if (proto == icmp && af == inet) {
} else if (proto == icmp6 && af == inet6) {
} else {
}
The current code would do icmp processing for an ipv6 packet with
protocol 1. Such a packet is strange but it should not get special
translation treatment.
bluhm
Re: carp shutdown in /etc/rc
On 5-2-2011 11:02, Henning Brauer wrote:
> on the other side, fixing "ifconfig very slow with lots of interfaces"
> deserves to be fixed anyway. looking at the code - either getifaddrs is
> slow (which in turn wouldn't be ifconfig only), or the ioctls ifconfig
> does in getinfo(). that's "just" 5 tho. wonder wether making one big
> ioctl that returns everything those 5 would help - wouldn't win a beauty
> price for sure. not that ifconfig would ever qualify.
If ifconfig.c is instrumented with a little perl script:
# cat ioctl_debug.pl
#!/usr/bin/perl -pi.orig
if (m{ioctl\(\w+, (\w+),}) {
my $sig = $1;
s{ioctl\(}{printf("ioctl $sig\\n") != -1 && ioctl(};
}
and then build like this:
# perl ioctl_debug.pl ifconfig.c
# make
# make install
you can see this:
# ifconfig carp80
ioctl SIOCGIFFLAGS
ioctl SIOCGIFXFLAGS
ioctl SIOCGIFMETRIC
ioctl SIOCGIFMTU
ioctl SIOCGIFRDOMAIN
ioctl SIOCGIFFLAGS
ioctl SIOCGIFXFLAGS
ioctl SIOCGIFMETRIC
ioctl SIOCGIFMTU
ioctl SIOCGIFRDOMAIN
carp80: flags=8843 mtu 1500
lladdr 00:00:5e:00:01:50
ioctl SIOCGIFDESCR
ioctl SIOCGIFPRIORITY
priority: 0
ioctl SIOCGETKALIVE
ioctl SIOCGETVLAN
ioctl SIOCGVH
carp: BACKUP carpdev vlan80 vhid 80 advbase 1 advskew 0
ioctl SIOCGETPFSYNC
ioctl PPPOEGETPARMS
ioctl SIOCGIFTIMESLOT
ioctl SIOCGIFGENERIC
ioctl SIOCGTRUNKPORT
ioctl SIOCGTRUNK
ioctl SIOCGETPFLOW
ioctl SIOCGIFGROUP
ioctl SIOCGIFGROUP
groups: carp
ioctl SIOCGIFMEDIA
status: backup
ioctl SIOCGLIFPHYADDR
inet6 fe80::200:5eff:fe00:150%carp80ioctl SIOCGIFNETMASK_IN6
prefixlen 64ioctl SIOCGIFAFLAG_IN6
scopeid 0x7ioctl SIOCGIFALIFETIME_IN6
inet 10.10.80.1ioctl SIOCGIFNETMASK
netmask 0xff00ioctl SIOCGIFBRDADDR
broadcast 10.10.80.255
# ifconfig vlan
ioctl SIOCGIFGMEMB
ioctl SIOCGIFGMEMB
ioctl SIOCGIFFLAGS
ioctl SIOCGIFXFLAGS
ioctl SIOCGIFMETRIC
ioctl SIOCGIFMTU
ioctl SIOCGIFRDOMAIN
vlan80: flags=8943 mtu 1500
lladdr 00:d0:59:b6:f4:27
ioctl SIOCGIFDESCR
ioctl SIOCGIFPRIORITY
priority: 0
ioctl SIOCGETKALIVE
ioctl SIOCGETVLAN
ioctl SIOCGETVLANPRIO
vlan: 80 priority: 0 parent interface: fxp0
ioctl SIOCGVH
ioctl SIOCGETPFSYNC
ioctl PPPOEGETPARMS
ioctl SIOCGIFTIMESLOT
ioctl SIOCGIFGENERIC
ioctl SIOCGTRUNKPORT
ioctl SIOCGTRUNK
ioctl SIOCGETPFLOW
ioctl SIOCGIFGROUP
ioctl SIOCGIFGROUP
groups: vlan
ioctl SIOCGIFMEDIA
status: active
ioctl SIOCGLIFPHYADDR
inet6 fe80::2d0:59ff:feb6:f427%vlan80ioctl SIOCGIFNETMASK_IN6
prefixlen 64ioctl SIOCGIFAFLAG_IN6
scopeid 0x6ioctl SIOCGIFALIFETIME_IN6
inet 10.10.80.2ioctl SIOCGIFNETMASK
netmask 0xff00ioctl SIOCGIFBRDADDR
broadcast 10.10.80.255
# ifconfig fxp
ioctl SIOCGIFGMEMB
ioctl SIOCGIFFLAGS
ioctl SIOCGIFXFLAGS
ioctl SIOCGIFMETRIC
ioctl SIOCGIFMTU
ioctl SIOCGIFRDOMAIN
fxp0:
flags=8b43 mtu 1500
lladdr 00:d0:59:b6:f4:27
ioctl SIOCGIFDESCR
ioctl SIOCGIFPRIORITY
priority: 0
ioctl SIOCGETKALIVE
ioctl SIOCGETVLAN
ioctl SIOCGVH
ioctl SIOCGETPFSYNC
ioctl PPPOEGETPARMS
ioctl SIOCGIFTIMESLOT
ioctl SIOCGIFGENERIC
ioctl SIOCGTRUNKPORT
ioctl SIOCGTRUNK
ioctl SIOCGETPFLOW
ioctl SIOCGIFGROUP
ioctl SIOCGIFGROUP
groups: egress
ioctl SIOCGIFMEDIA
ioctl SIOCGIFMEDIA
media: Ethernet autoselect (100baseTX full-duplex)
status: active
ioctl SIOCG80211NWID
ioctl SIOCG80211NWKEY
ioctl SIOCG80211WPAPSK
ioctl SIOCG80211POWER
ioctl SIOCG80211CHANNEL
ioctl SIOCG80211BSSID
ioctl SIOCG80211TXPOWER
ioctl SIOCG80211WPAPARMS
ioctl SIOCGLIFPHYADDR
inet6 fe80::2d0:59ff:feb6:f427%fxp0ioctl SIOCGIFNETMASK_IN6
prefixlen 64ioctl SIOCGIFAFLAG_IN6
scopeid 0x2ioctl SIOCGIFALIFETIME_IN6
inet 192.168.28.129ioctl SIOCGIFNETMASK
netmask 0xff00ioctl SIOCGIFBRDADDR
broadcast 192.168.28.255
So yeah, it looks like ifconfig can be made a little smarter.
Re: carp shutdown in /etc/rc
* Camiel Dobbelaar [2011-02-05 10:12]: > No, it's ifconfig itself that takes long. as said privately, i'd be surprised if it isn't the gazillion ioctls ifconfig does. > It does not scale linearly, but the real world usage (200) is fine. I > think we can drop the diff, since it turned out not to be so obvious and > clean... now... we DO take down ALL interfaces on shutdown now. equivalent to ifconfig down. that was the result of an interesting bug i ran into in stockholm, i was rebooting a soekris over and over and every now and then had weird panics. turned out that the dma engine of the NICs was scribbling over my memory since we didn't stop them at shutdown. so this code in rc isn't all that much needed any more - now it "just" makes sure the carp ifs are taken down before the real ones. that is still good to have. now i wonder wether we can do this kernel side early enough before the other interfaces go so that there is sufficient headroom for a hitless failover. on the other side, fixing "ifconfig very slow with lots of interfaces" deserves to be fixed anyway. looking at the code - either getifaddrs is slow (which in turn wouldn't be ifconfig only), or the ioctls ifconfig does in getinfo(). that's "just" 5 tho. wonder wether making one big ioctl that returns everything those 5 would help - wouldn't win a beauty price for sure. not that ifconfig would ever qualify. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services, http://bsws.de Full-Service ISP - Secure Hosting, Mail and DNS Services Dedicated Servers, Rootservers, Application Hosting
Unlock your Wii todday!
Get the Wii UNLOCKER ULTRA GUIDE Complete with 5 High Definition Video Tutorials That take you by the hand until you unlock your Wii without losing your warranty. Plus you get five cool bonuses that take your Wii to a whole new level of entertainment. Here is what you can do with your Wii after you unlocked it: * Play Backed up copies of your Games * Play Imported Games * Use Your Wii As a DVD Player * Listen to Radio right from your Wii * And download and play your favorite homebrew Games Click here to enjoy your unlocked Wii Best regards __ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
Re: carp shutdown in /etc/rc
On 5-2-2011 2:15, Ted Unangst wrote: > On Fri, Feb 4, 2011 at 7:21 AM, Camiel Dobbelaar wrote: >> With hundreds of (vlan) interfaces, a shutdown takes quite a while. >># bring carp interfaces down gracefully >> - ifconfig | while read a b; do >> + ifconfig carp | while read a b; do > > going back to the original issue, does "ifconfig | grep carp | while > read a b" make things faster? No, it's ifconfig itself that takes long. With 2000 vlan interfaces and 1 carp interface: # time ifconfig | grep ^carp carp80: flags=8843 mtu 1500 1m11.29s real 0m12.07s user 0m59.03s system # time ifconfig carp | grep ^carp carp80: flags=8843 mtu 1500 0m0.06s real 0m0.01s user 0m0.05s system 1000 # time ifconfig | grep ^carp carp80: flags=8843 mtu 1500 0m16.66s real 0m2.88s user 0m13.72s system # time ifconfig carp | grep ^carp carp80: flags=8843 mtu 1500 0m0.03s real 0m0.00s user 0m0.02s system 500 # time ifconfig | grep ^carp carp80: flags=8843 mtu 1500 0m3.18s real 0m0.67s user 0m2.49s system # time ifconfig carp | grep ^carp carp80: flags=8843 mtu 1500 0m0.02s real 0m0.00s user 0m0.01s system 200 # time ifconfig | grep ^carp carp80: flags=8843 mtu 1500 0m0.35s real 0m0.07s user 0m0.27s system # time ifconfig carp | grep ^carp carp80: flags=8843 mtu 1500 0m0.01s real 0m0.00s user 0m0.00s system It does not scale linearly, but the real world usage (200) is fine. I think we can drop the diff, since it turned out not to be so obvious and clean... -- Cam
