* [B.A.T.M.A.N.] [PATCH] batman-adv: check return value for hash_add()
@ 2011-11-02 9:42 Simon Wunderlich
2011-11-02 10:13 ` Antonio Quartulli
0 siblings, 1 reply; 3+ messages in thread
From: Simon Wunderlich @ 2011-11-02 9:42 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Simon Wunderlich
if hash_add() fails, we should remove the structure to avoid memory
leaks.
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
---
As this was the last unchecked occurence, this should close bug #136
in the open-mesh.org redmine bugtracker.
---
translation-table.c | 22 ++++++++++++++++++----
1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/translation-table.c b/translation-table.c
index 5b60aba..1390179 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -190,6 +190,7 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
struct bat_priv *bat_priv = netdev_priv(soft_iface);
struct tt_local_entry *tt_local_entry = NULL;
struct tt_global_entry *tt_global_entry = NULL;
+ int hash_added;
tt_local_entry = tt_local_hash_find(bat_priv, addr);
@@ -217,16 +218,22 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
if (compare_eth(addr, soft_iface->dev_addr))
tt_local_entry->common.flags |= TT_CLIENT_NOPURGE;
- tt_local_event(bat_priv, addr, tt_local_entry->common.flags);
-
/* The local entry has to be marked as NEW to avoid to send it in
* a full table response going out before the next ttvn increment
* (consistency check) */
tt_local_entry->common.flags |= TT_CLIENT_NEW;
- hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
+ hash_added = hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
&tt_local_entry->common, &tt_local_entry->common.hash_entry);
+ if (unlikely(!hash_added)) {
+ /* remove the reference for the hash */
+ tt_local_entry_free_ref(tt_local_entry);
+ goto out;
+ }
+
+ tt_local_event(bat_priv, addr, tt_local_entry->common.flags);
+
/* remove address from global hash if present */
tt_global_entry = tt_global_hash_find(bat_priv, addr);
@@ -499,6 +506,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
struct tt_global_entry *tt_global_entry;
struct orig_node *orig_node_tmp;
int ret = 0;
+ int hash_added;
tt_global_entry = tt_global_hash_find(bat_priv, tt_addr);
@@ -518,9 +526,15 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
tt_global_entry->ttvn = ttvn;
tt_global_entry->roam_at = 0;
- hash_add(bat_priv->tt_global_hash, compare_tt,
+ hash_added = hash_add(bat_priv->tt_global_hash, compare_tt,
choose_orig, &tt_global_entry->common,
&tt_global_entry->common.hash_entry);
+
+ if (unlikely(!hash_added)) {
+ /* remove the reference for the hash */
+ tt_global_entry_free_ref(tt_global_entry);
+ goto out;
+ }
atomic_inc(&orig_node->tt_size);
} else {
if (tt_global_entry->orig_node != orig_node) {
--
1.7.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batman-adv: check return value for hash_add()
2011-11-02 9:42 [B.A.T.M.A.N.] [PATCH] batman-adv: check return value for hash_add() Simon Wunderlich
@ 2011-11-02 10:13 ` Antonio Quartulli
2011-11-02 19:23 ` Simon Wunderlich
0 siblings, 1 reply; 3+ messages in thread
From: Antonio Quartulli @ 2011-11-02 10:13 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
Cc: Simon Wunderlich
Hello Simon,
On Wed, Nov 02, 2011 at 10:42:49 +0100, Simon Wunderlich wrote:
> if hash_add() fails, we should remove the structure to avoid memory
> leaks.
>
> Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> ---
> As this was the last unchecked occurence, this should close bug #136
> in the open-mesh.org redmine bugtracker.
> ---
> translation-table.c | 22 ++++++++++++++++++----
> 1 files changed, 18 insertions(+), 4 deletions(-)
>
> @@ -217,16 +218,22 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
> - hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
> + hash_added = hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
> &tt_local_entry->common, &tt_local_entry->common.hash_entry);
In my opinion you should indent this invocation a bit better :-P
>
> + if (unlikely(!hash_added)) {
> + /* remove the reference for the hash */
> + tt_local_entry_free_ref(tt_local_entry);
> + goto out;
> + }
> +
> + tt_local_event(bat_priv, addr, tt_local_entry->common.flags);
> +
Here you are invoking tt_local_event() _after_ setting the TT_CLIENT_NEW. This
means that this flag is going to be copied in the tt_change structure and later
it will be sent out within the next OGM. This has to not happen. Therefore I would
move the or-assignment of the TT_CLIENT_NEW flag _after_ the invocation of
tt_local_event().
> /* remove address from global hash if present */
> tt_global_entry = tt_global_hash_find(bat_priv, addr);
>
> @@ -499,6 +506,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
> struct tt_global_entry *tt_global_entry;
> struct orig_node *orig_node_tmp;
> int ret = 0;
> + int hash_added;
>
> tt_global_entry = tt_global_hash_find(bat_priv, tt_addr);
>
> @@ -518,9 +526,15 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
> tt_global_entry->ttvn = ttvn;
> tt_global_entry->roam_at = 0;
>
> - hash_add(bat_priv->tt_global_hash, compare_tt,
> + hash_added = hash_add(bat_priv->tt_global_hash, compare_tt,
> choose_orig, &tt_global_entry->common,
> &tt_global_entry->common.hash_entry);
> +
> + if (unlikely(!hash_added)) {
> + /* remove the reference for the hash */
> + tt_global_entry_free_ref(tt_global_entry);
> + goto out;
> + }
In this case the global client could be a roaming one....what about invoking
tt_local_remove() first? Otherwise we would still have a local client which is
actually not one of ours..What do you think? Moreover it would free some space
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batman-adv: check return value for hash_add()
2011-11-02 10:13 ` Antonio Quartulli
@ 2011-11-02 19:23 ` Simon Wunderlich
0 siblings, 0 replies; 3+ messages in thread
From: Simon Wunderlich @ 2011-11-02 19:23 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking
[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]
On Wed, Nov 02, 2011 at 11:13:57AM +0100, Antonio Quartulli wrote:
> Hello Simon,
>
> On Wed, Nov 02, 2011 at 10:42:49 +0100, Simon Wunderlich wrote:
> > if hash_add() fails, we should remove the structure to avoid memory
> > leaks.
> >
> > Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> > ---
> > As this was the last unchecked occurence, this should close bug #136
> > in the open-mesh.org redmine bugtracker.
> > ---
> > translation-table.c | 22 ++++++++++++++++++----
> > 1 files changed, 18 insertions(+), 4 deletions(-)
> >
> > @@ -217,16 +218,22 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
> > - hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
> > + hash_added = hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
> > &tt_local_entry->common, &tt_local_entry->common.hash_entry);
>
> In my opinion you should indent this invocation a bit better :-P
>
OK
> >
> > + if (unlikely(!hash_added)) {
> > + /* remove the reference for the hash */
> > + tt_local_entry_free_ref(tt_local_entry);
> > + goto out;
> > + }
> > +
> > + tt_local_event(bat_priv, addr, tt_local_entry->common.flags);
> > +
>
> Here you are invoking tt_local_event() _after_ setting the TT_CLIENT_NEW. This
> means that this flag is going to be copied in the tt_change structure and later
> it will be sent out within the next OGM. This has to not happen. Therefore I would
> move the or-assignment of the TT_CLIENT_NEW flag _after_ the invocation of
> tt_local_event().
>
>
OK, i will move it.
> > /* remove address from global hash if present */
> > tt_global_entry = tt_global_hash_find(bat_priv, addr);
> >
> > @@ -499,6 +506,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
> > struct tt_global_entry *tt_global_entry;
> > struct orig_node *orig_node_tmp;
> > int ret = 0;
> > + int hash_added;
> >
> > tt_global_entry = tt_global_hash_find(bat_priv, tt_addr);
> >
> > @@ -518,9 +526,15 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
> > tt_global_entry->ttvn = ttvn;
> > tt_global_entry->roam_at = 0;
> >
> > - hash_add(bat_priv->tt_global_hash, compare_tt,
> > + hash_added = hash_add(bat_priv->tt_global_hash, compare_tt,
> > choose_orig, &tt_global_entry->common,
> > &tt_global_entry->common.hash_entry);
> > +
> > + if (unlikely(!hash_added)) {
> > + /* remove the reference for the hash */
> > + tt_global_entry_free_ref(tt_global_entry);
> > + goto out;
> > + }
>
> In this case the global client could be a roaming one....what about invoking
> tt_local_remove() first? Otherwise we would still have a local client which is
> actually not one of ours..What do you think? Moreover it would free some space
Sure, we can do that.
I'll send a v2 patch in a minute ...
thanks
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-11-02 19:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-02 9:42 [B.A.T.M.A.N.] [PATCH] batman-adv: check return value for hash_add() Simon Wunderlich
2011-11-02 10:13 ` Antonio Quartulli
2011-11-02 19:23 ` Simon Wunderlich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox