Re: [PATCH] Fix ospfd segmentation fault on startup
commited, thx for your diff.
/Benno
Johan Ymerson(johan.ymer...@transmode.com) on 2015.07.20 21:32:20 +:
> On Mon, 2015-07-20 at 22:58 +0200, Martin Pieuchot wrote:
> > On 20/07/15(Mon) 19:10, Johan Ymerson wrote:
> > > On 2015-07-18 16:03:00, Martin Pieuchot wrote:
> > > > Committed! Thanks and sorry for the delay.
> > >
> > > Hi!
> > >
> > > You missed the previous patch "Fix ospfd segmentation fault on startup"
> > > witch prevent ospfd from segfaulting on startup. Without this first
> > > patch, ospfd will almost always segfault on startup (instead of just
> > > sometime, which it does today).
> >
> > Could you send a single diff for all these issues? Apparently ospf
> > hackers are slacking ;)
>
> Yes, I have it as a single diff. I actually broke it up in two diffs
> because the two issues are not really related. The second patch only
> makes the first problem very obvious.
> It would of course be best if we could make sure we set up event
> handling (iev_ospfe et al.) before scanning interfaces, but it's kind of
> a catch 22 here. The event handlers need the interface info to not
> segfault... So the easy fix was to just check for null pointers in
> main_imsg_compose_*.
>
> /Johan
>
>
>
> Index: interface.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
> retrieving revision 1.75
> diff -u -p -r1.75 interface.c
> --- interface.c 14 May 2012 10:17:21 - 1.75
> +++ interface.c 27 May 2015 16:42:51 -
> @@ -338,8 +338,10 @@ if_act_start(struct iface *iface)
> struct in_addr addr;
> struct timeval now;
>
> - if (!((iface->flags & IFF_UP) &&
> - LINK_STATE_IS_UP(iface->linkstate)))
> + if (!(iface->flags & IFF_UP) ||
> + (!LINK_STATE_IS_UP(iface->linkstate) &&
> + !(iface->media_type == IFT_CARP &&
> + iface->linkstate == LINK_STATE_DOWN)))
> return (0);
>
> if (iface->media_type == IFT_CARP && iface->passive == 0) {
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 kroute.c
> --- kroute.c11 Feb 2015 05:57:44 - 1.98
> +++ kroute.c27 May 2015 16:42:51 -
> @@ -1019,6 +1019,9 @@ if_change(u_short ifindex, int flags, st
> return;
> }
>
> + /* notify ospfe about interface link state */
> + main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
> +
> reachable = (kif->flags & IFF_UP) &&
> LINK_STATE_IS_UP(kif->link_state);
>
> @@ -1026,9 +1029,6 @@ if_change(u_short ifindex, int flags, st
> return; /* nothing changed wrt nexthop validity */
>
> kif->nh_reachable = reachable;
> -
> - /* notify ospfe about interface link state */
> - main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
>
> /* update redistribute list */
> RB_FOREACH(kr, kroute_tree, &krt) {
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.83
> diff -u -p -r1.83 ospfd.c
> --- ospfd.c 10 Feb 2015 05:24:48 - 1.83
> +++ ospfd.c 27 May 2015 16:42:51 -
> @@ -511,13 +511,15 @@ main_dispatch_rde(int fd, short event, v
> void
> main_imsg_compose_ospfe(int type, pid_t pid, void *data, u_int16_t datalen)
> {
> - imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
> + if (iev_ospfe)
> + imsg_compose_event(iev_ospfe, type, 0, pid, -1, data,
> datalen);
> }
>
> void
> main_imsg_compose_rde(int type, pid_t pid, void *data, u_int16_t datalen)
> {
> - imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
> + if (iev_rde)
> + imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
> }
>
> void
>
--
Re: [PATCH] Fix ospfd segmentation fault on startup
Johan Ymerson(johan.ymer...@transmode.com) on 2015.07.20 21:32:20 +:
> On Mon, 2015-07-20 at 22:58 +0200, Martin Pieuchot wrote:
> > On 20/07/15(Mon) 19:10, Johan Ymerson wrote:
> > > On 2015-07-18 16:03:00, Martin Pieuchot wrote:
> > > > Committed! Thanks and sorry for the delay.
> > >
> > > Hi!
> > >
> > > You missed the previous patch "Fix ospfd segmentation fault on startup"
> > > witch prevent ospfd from segfaulting on startup. Without this first
> > > patch, ospfd will almost always segfault on startup (instead of just
> > > sometime, which it does today).
> >
> > Could you send a single diff for all these issues? Apparently ospf
> > hackers are slacking ;)
>
> Yes, I have it as a single diff. I actually broke it up in two diffs
> because the two issues are not really related. The second patch only
> makes the first problem very obvious.
> It would of course be best if we could make sure we set up event
> handling (iev_ospfe et al.) before scanning interfaces, but it's kind of
> a catch 22 here. The event handlers need the interface info to not
> segfault... So the easy fix was to just check for null pointers in
> main_imsg_compose_*.
>
> /Johan
fixed it.
ok benno@
>
>
>
> Index: interface.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
> retrieving revision 1.75
> diff -u -p -r1.75 interface.c
> --- interface.c 14 May 2012 10:17:21 - 1.75
> +++ interface.c 27 May 2015 16:42:51 -
> @@ -338,8 +338,10 @@ if_act_start(struct iface *iface)
> struct in_addr addr;
> struct timeval now;
>
> - if (!((iface->flags & IFF_UP) &&
> - LINK_STATE_IS_UP(iface->linkstate)))
> + if (!(iface->flags & IFF_UP) ||
> + (!LINK_STATE_IS_UP(iface->linkstate) &&
> + !(iface->media_type == IFT_CARP &&
> + iface->linkstate == LINK_STATE_DOWN)))
> return (0);
>
> if (iface->media_type == IFT_CARP && iface->passive == 0) {
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 kroute.c
> --- kroute.c11 Feb 2015 05:57:44 - 1.98
> +++ kroute.c27 May 2015 16:42:51 -
> @@ -1019,6 +1019,9 @@ if_change(u_short ifindex, int flags, st
> return;
> }
>
> + /* notify ospfe about interface link state */
> + main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
> +
> reachable = (kif->flags & IFF_UP) &&
> LINK_STATE_IS_UP(kif->link_state);
>
> @@ -1026,9 +1029,6 @@ if_change(u_short ifindex, int flags, st
> return; /* nothing changed wrt nexthop validity */
>
> kif->nh_reachable = reachable;
> -
> - /* notify ospfe about interface link state */
> - main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
>
> /* update redistribute list */
> RB_FOREACH(kr, kroute_tree, &krt) {
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.83
> diff -u -p -r1.83 ospfd.c
> --- ospfd.c 10 Feb 2015 05:24:48 - 1.83
> +++ ospfd.c 27 May 2015 16:42:51 -
> @@ -511,13 +511,15 @@ main_dispatch_rde(int fd, short event, v
> void
> main_imsg_compose_ospfe(int type, pid_t pid, void *data, u_int16_t datalen)
> {
> - imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
> + if (iev_ospfe)
> + imsg_compose_event(iev_ospfe, type, 0, pid, -1, data,
> datalen);
> }
>
> void
> main_imsg_compose_rde(int type, pid_t pid, void *data, u_int16_t datalen)
> {
> - imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
> + if (iev_rde)
> + imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
> }
>
> void
>
--
Re: [PATCH] Fix ospfd segmentation fault on startup
On Mon, Jul 20, 2015 at 09:32:20PM +, Johan Ymerson wrote:
> On Mon, 2015-07-20 at 22:58 +0200, Martin Pieuchot wrote:
> > On 20/07/15(Mon) 19:10, Johan Ymerson wrote:
> > > On 2015-07-18 16:03:00, Martin Pieuchot wrote:
> > > > Committed! Thanks and sorry for the delay.
> > >
> > > Hi!
> > >
> > > You missed the previous patch "Fix ospfd segmentation fault on startup"
> > > witch prevent ospfd from segfaulting on startup. Without this first
> > > patch, ospfd will almost always segfault on startup (instead of just
> > > sometime, which it does today).
> >
> > Could you send a single diff for all these issues? Apparently ospf
> > hackers are slacking ;)
>
> Yes, I have it as a single diff. I actually broke it up in two diffs
> because the two issues are not really related. The second patch only
> makes the first problem very obvious.
> It would of course be best if we could make sure we set up event
> handling (iev_ospfe et al.) before scanning interfaces, but it's kind of
> a catch 22 here. The event handlers need the interface info to not
> segfault... So the easy fix was to just check for null pointers in
> main_imsg_compose_*.
>
> /Johan
>
Yes, lets go with this version. OK claudio@
>
> Index: interface.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
> retrieving revision 1.75
> diff -u -p -r1.75 interface.c
> --- interface.c 14 May 2012 10:17:21 - 1.75
> +++ interface.c 27 May 2015 16:42:51 -
> @@ -338,8 +338,10 @@ if_act_start(struct iface *iface)
> struct in_addr addr;
> struct timeval now;
>
> - if (!((iface->flags & IFF_UP) &&
> - LINK_STATE_IS_UP(iface->linkstate)))
> + if (!(iface->flags & IFF_UP) ||
> + (!LINK_STATE_IS_UP(iface->linkstate) &&
> + !(iface->media_type == IFT_CARP &&
> + iface->linkstate == LINK_STATE_DOWN)))
> return (0);
>
> if (iface->media_type == IFT_CARP && iface->passive == 0) {
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 kroute.c
> --- kroute.c11 Feb 2015 05:57:44 - 1.98
> +++ kroute.c27 May 2015 16:42:51 -
> @@ -1019,6 +1019,9 @@ if_change(u_short ifindex, int flags, st
> return;
> }
>
> + /* notify ospfe about interface link state */
> + main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
> +
> reachable = (kif->flags & IFF_UP) &&
> LINK_STATE_IS_UP(kif->link_state);
>
> @@ -1026,9 +1029,6 @@ if_change(u_short ifindex, int flags, st
> return; /* nothing changed wrt nexthop validity */
>
> kif->nh_reachable = reachable;
> -
> - /* notify ospfe about interface link state */
> - main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
>
> /* update redistribute list */
> RB_FOREACH(kr, kroute_tree, &krt) {
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.83
> diff -u -p -r1.83 ospfd.c
> --- ospfd.c 10 Feb 2015 05:24:48 - 1.83
> +++ ospfd.c 27 May 2015 16:42:51 -
> @@ -511,13 +511,15 @@ main_dispatch_rde(int fd, short event, v
> void
> main_imsg_compose_ospfe(int type, pid_t pid, void *data, u_int16_t datalen)
> {
> - imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
> + if (iev_ospfe)
> + imsg_compose_event(iev_ospfe, type, 0, pid, -1, data,
> datalen);
> }
>
> void
> main_imsg_compose_rde(int type, pid_t pid, void *data, u_int16_t datalen)
> {
> - imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
> + if (iev_rde)
> + imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
> }
>
> void
>
--
:wq Claudio
Re: [PATCH] Fix ospfd segmentation fault on startup
On Mon, 2015-07-20 at 22:58 +0200, Martin Pieuchot wrote:
> On 20/07/15(Mon) 19:10, Johan Ymerson wrote:
> > On 2015-07-18 16:03:00, Martin Pieuchot wrote:
> > > Committed! Thanks and sorry for the delay.
> >
> > Hi!
> >
> > You missed the previous patch "Fix ospfd segmentation fault on startup"
> > witch prevent ospfd from segfaulting on startup. Without this first
> > patch, ospfd will almost always segfault on startup (instead of just
> > sometime, which it does today).
>
> Could you send a single diff for all these issues? Apparently ospf
> hackers are slacking ;)
Yes, I have it as a single diff. I actually broke it up in two diffs
because the two issues are not really related. The second patch only
makes the first problem very obvious.
It would of course be best if we could make sure we set up event
handling (iev_ospfe et al.) before scanning interfaces, but it's kind of
a catch 22 here. The event handlers need the interface info to not
segfault... So the easy fix was to just check for null pointers in
main_imsg_compose_*.
/Johan
Index: interface.c
===
RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
retrieving revision 1.75
diff -u -p -r1.75 interface.c
--- interface.c 14 May 2012 10:17:21 - 1.75
+++ interface.c 27 May 2015 16:42:51 -
@@ -338,8 +338,10 @@ if_act_start(struct iface *iface)
struct in_addr addr;
struct timeval now;
- if (!((iface->flags & IFF_UP) &&
- LINK_STATE_IS_UP(iface->linkstate)))
+ if (!(iface->flags & IFF_UP) ||
+ (!LINK_STATE_IS_UP(iface->linkstate) &&
+ !(iface->media_type == IFT_CARP &&
+ iface->linkstate == LINK_STATE_DOWN)))
return (0);
if (iface->media_type == IFT_CARP && iface->passive == 0) {
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
retrieving revision 1.98
diff -u -p -r1.98 kroute.c
--- kroute.c11 Feb 2015 05:57:44 - 1.98
+++ kroute.c27 May 2015 16:42:51 -
@@ -1019,6 +1019,9 @@ if_change(u_short ifindex, int flags, st
return;
}
+ /* notify ospfe about interface link state */
+ main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
+
reachable = (kif->flags & IFF_UP) &&
LINK_STATE_IS_UP(kif->link_state);
@@ -1026,9 +1029,6 @@ if_change(u_short ifindex, int flags, st
return; /* nothing changed wrt nexthop validity */
kif->nh_reachable = reachable;
-
- /* notify ospfe about interface link state */
- main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
/* update redistribute list */
RB_FOREACH(kr, kroute_tree, &krt) {
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.83
diff -u -p -r1.83 ospfd.c
--- ospfd.c 10 Feb 2015 05:24:48 - 1.83
+++ ospfd.c 27 May 2015 16:42:51 -
@@ -511,13 +511,15 @@ main_dispatch_rde(int fd, short event, v
void
main_imsg_compose_ospfe(int type, pid_t pid, void *data, u_int16_t datalen)
{
- imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
+ if (iev_ospfe)
+ imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
}
void
main_imsg_compose_rde(int type, pid_t pid, void *data, u_int16_t datalen)
{
- imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
+ if (iev_rde)
+ imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
}
void
Re: [PATCH] Fix ospfd segmentation fault on startup
On 20/07/15(Mon) 19:10, Johan Ymerson wrote: > On 2015-07-18 16:03:00, Martin Pieuchot wrote: > > Committed! Thanks and sorry for the delay. > > Hi! > > You missed the previous patch "Fix ospfd segmentation fault on startup" > witch prevent ospfd from segfaulting on startup. Without this first > patch, ospfd will almost always segfault on startup (instead of just > sometime, which it does today). Could you send a single diff for all these issues? Apparently ospf hackers are slacking ;)
Re: [PATCH] Fix ospfd segmentation fault on startup
On 2015-07-18 16:03:00, Martin Pieuchot wrote: > Committed! Thanks and sorry for the delay. Hi! You missed the previous patch "Fix ospfd segmentation fault on startup" witch prevent ospfd from segfaulting on startup. Without this first patch, ospfd will almost always segfault on startup (instead of just sometime, which it does today). /Johan
[PATCH] Fix ospfd segmentation fault on startup
Hi,
When debugging problems with ospfd and carp on startup, I managed to get
ospfd to segfault a couple of times.
I tracked down the issue to if_change() and main_imsg_compose_ospfe().
if_change() is called before imsg_init is called to initialize the
imsgbuf struct. If a link state change to UP occurs during the small
time frame the imsgbuf pointer is uninitialized, we have a null pointer
dereference in main_imsg_compose_ospfe().
Safe-guard against this by simply not calling imsg_compose_event() if
the imsgbuf pointer is null.
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.83
diff -u -p -r1.83 ospfd.c
--- ospfd.c 10 Feb 2015 05:24:48 - 1.83
+++ ospfd.c 27 May 2015 12:35:08 -
@@ -511,13 +511,15 @@ main_dispatch_rde(int fd, short event, v
void
main_imsg_compose_ospfe(int type, pid_t pid, void *data, u_int16_t datalen)
{
- imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
+ if (iev_ospfe)
+ imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
}
void
main_imsg_compose_rde(int type, pid_t pid, void *data, u_int16_t datalen)
{
- imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
+ if (iev_rde)
+ imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
}
void
