From: Pablo Neira Ayuso pablo@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; }
pablo@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
Peter Stuge 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.
A break; would also be fine I think. Anything to make the flow explicit.
//Peter
Hi Peter,
On Tue, Oct 16, 2012 at 06:46:05PM +0200, Peter Stuge wrote:
pablo@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.
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.
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.
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:
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.
Will do. I'll change that break as you want it before doing so.
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.
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
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
On Thu, Oct 18, 2012 at 07:16:43PM +0200, Peter Stuge wrote:
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!
Never mind, I already pushed it including the break thing :-)
-
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org
-
Peter Stuge