16 Oct
2012
16 Oct
'12
7:23 p.m.
From: Pablo Neira Ayuso <pablo(a)gnumonks.org>
This patch fixes several leak of msgbs in uncommon error paths.
---
openbsc/src/libbsc/abis_nm.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/openbsc/src/libbsc/abis_nm.c b/openbsc/src/libbsc/abis_nm.c
index ee0dd60..2c8b3ee 100644
--- a/openbsc/src/libbsc/abis_nm.c
+++ b/openbsc/src/libbsc/abis_nm.c
@@ -635,13 +635,16 @@ int abis_nm_rcvmsg(struct msgb *msg)
if (oh->placement != ABIS_OM_PLACEMENT_ONLY) {
LOGP(DNM, LOGL_ERROR, "ABIS OML placement 0x%x not supported\n",
oh->placement);
- if (oh->placement != ABIS_OM_PLACEMENT_FIRST)
- return -EINVAL;
+ if (oh->placement != ABIS_OM_PLACEMENT_FIRST) {
+ rc = -EINVAL;
+ goto err;
+ }
}
if (oh->sequence != 0) {
LOGP(DNM, LOGL_ERROR, "ABIS OML sequence 0x%x != 0x00\n",
oh->sequence);
- return -EINVAL;
+ rc = -EINVAL;
+ goto err;
}
#if 0
unsigned int l2_len = msg->tail - (uint8_t *)msgb_l2(msg);
@@ -671,9 +674,9 @@ int abis_nm_rcvmsg(struct msgb *msg)
default:
LOGP(DNM, LOGL_ERROR, "unknown ABIS OML message discriminator 0x%x\n",
oh->mdisc);
- return -EINVAL;
+ rc = -EINVAL;
}
-
+err:
msgb_free(msg);
return rc;
}
--
1.7.10.4
16 Oct
16 Oct
7:46 p.m.
pablo(a)gnumonks.org wrote:
@@ -671,9 +674,9 @@ int abis_nm_rcvmsg(struct msgb
*msg)
default:
LOGP(DNM, LOGL_ERROR, "unknown ABIS OML message discriminator 0x%x\n",
oh->mdisc);
- return -EINVAL;
+ rc = -EINVAL;
}
It looks like this is at the end of a switch () statement, so maybe
it is a good idea to include a goto statement for clarity, even if
that is not absolutely neccessary given how the code looks right now.
-
+err:
msgb_free(msg);
return rc;
}
Since the label is not only an error path but also reached during
successful flow through the function I suggest to name it something
like "done" instead.
Thanks a lot for fixing these leaks!
//Peter
7:53 p.m.
Peter Stuge wrote:
A break; would also be fine I think. Anything to make the flow explicit.
//Peter
@@ -671,9
+674,9 @@ int abis_nm_rcvmsg(struct msgb *msg)
default:
LOGP(DNM, LOGL_ERROR, "unknown ABIS OML message discriminator 0x%x\n",
oh->mdisc);
- return -EINVAL;
+ rc = -EINVAL;
}
It looks like this is at the end of a switch () statement, so maybe
it is a good idea to include a goto statement for clarity, even if
that is not absolutely neccessary given how the code looks right now.
18 Oct
18 Oct
3:19 p.m.
Hi Peter,
On Tue, Oct 16, 2012 at 06:46:05PM +0200, Peter Stuge wrote:
pablo(a)gnumonks.org wrote:
at the risk of being annoying, I think the patch is good as is.
I don't want that this becomes a "bikeshed color" discussion [1], I
think the code changes are clear enough.
Thanks for your feedback.
[1] http://bikeshed.com/
@@ -671,9 +674,9 @@ int abis_nm_rcvmsg(struct
msgb *msg)
default:
LOGP(DNM, LOGL_ERROR, "unknown ABIS OML message discriminator 0x%x\n",
oh->mdisc);
- return -EINVAL;
+ rc = -EINVAL;
}
It looks like this is at the end of a switch () statement, so maybe
it is a good idea to include a goto statement for clarity, even if
that is not absolutely neccessary given how the code looks right now.
-
+err:
msgb_free(msg);
return rc;
}
Since the label is not only an error path but also reached during
successful flow through the function I suggest to name it something
like "done" instead. 
4:09 p.m.
On Thu, Oct 18, 2012 at 02:19:43PM +0200, Pablo Neira Ayuso wrote:
I don't want that this becomes a "bikeshed
color" discussion [1], I
think the code changes are clear enough.
well, there is no way I build a nuclear power plant so the bikeshed
is all I have. :)
I kind of agree with peter that a 'break' in the default would be
nice. It is just one of the habbits (including me writing code like
return X;break;).
Anyway, feel free to commit the leak fixes.
PS: I find it amusing how after a month suddenly no day passes without
someone saying his nanoBTS is dropped. So I would like to keep the
'dropping' until someone feels encouraged enough to create a PCAP file
and we fix the 'bad' return statement.
10:09 p.m.
On Thu, Oct 18, 2012 at 03:09:10PM +0200, Holger Hans Peter Freyther wrote:
On Thu, Oct 18, 2012 at 02:19:43PM +0200, Pablo Neira
Ayuso wrote:
Will do. I'll change that break as you want it before doing so.
I don't want that this becomes a
"bikeshed color" discussion [1], I
think the code changes are clear enough.
well, there is no way I build a nuclear power plant so the bikeshed
is all I have. :)
I kind of agree with peter that a 'break' in the default would be
nice. It is just one of the habbits (including me writing code like
return X;break;).
Anyway, feel free to commit the leak fixes. PS: I find it amusing how after a month suddenly no
day passes without
someone saying his nanoBTS is dropped. So I would like to keep the
'dropping' until someone feels encouraged enough to create a PCAP file
and we fix the 'bad' return statement.
I pushed the patch to libosmo-abis. I can revert it if you want, or
feel free to make it yourself, of course.
11:47 p.m.
On Thu, Oct 18, 2012 at 09:09:25PM +0200, Pablo Neira Ayuso wrote:
I pushed the patch to libosmo-abis. I can revert it if
you want, or
feel free to make it yourself, of course.
Reverting would is too drastic, it just makes me sad that none of the one
2-4 people that experienced this went to the bottom of the problem. We
might have some message we should handle, or need to improve the returns
in the nitb.
holger
8:16 p.m.
Pablo Neira Ayuso wrote:
at the risk of being annoying, I think the patch is
good as is.
I don't want that this becomes a "bikeshed color" discussion [1],
I think the code changes are clear enough.
The patch is clear and a great improvement. The code in the file
*after patching* could be made more clear with only the minimal
changes I mentioned. Of course it is subjective what is clear and
not. :)
I'm happy to send a follow-up patch once you've plugged the leak!
//Peter
19 Oct
19 Oct
12:40 a.m.
On Thu, Oct 18, 2012 at 07:16:43PM +0200, Peter Stuge wrote:
Pablo Neira Ayuso wrote:
Never mind, I already pushed it including the break thing :-)
at the risk of being annoying, I think the patch
is good as is.
I don't want that this becomes a "bikeshed color" discussion [1],
I think the code changes are clear enough.
The patch is clear and a great improvement. The code in the file
*after patching* could be made more clear with only the minimal
changes I mentioned. Of course it is subjective what is clear and
not. :)
I'm happy to send a follow-up patch once you've plugged the leak!
4244
days inactive
4246
days old
8 comments
4 participants
participants (4)
-
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org
-
Peter Stuge