10 Aug
2010
10 Aug
'10
10:24 p.m.
Hi all,
I was debugging a crash with the VTY and I am not fully understanding it but I
can easily reproduce it. The problem is triggered with the new VTY OML code
and here is what I know.
1.) we move from ENABLE_NODE to the OML node. and allocate memory
2.) when exiting the node we will free the data at vty->index.. and
set the pointer to NULL.
But I can easily produce a double free issue and this seems to be due.
cmd_execute_command:
1.) saves the vty->index to oindex and vty->node to onode
2.) tries calling vty_go_parent or such...
3.) tries more stuff...
4.) as stuff failed resets vty->index to oindex...
so the next exit command will do a double free... and the funny part is that
the code has one assumption everything > CONFIG_NODE is considered to be
config as well. This means that we should have two enum values in libosmovty,
one for Last_Enable, one for Last_Config and have enough space between them.
And the other part... maybe vty_go_parent should return CMD_SUCCESS?
thoughts?
[1] ./script | telnet localhost 4242 and see the bsc_hack/bsc_msc_io crash
10 Aug
10 Aug
10:39 p.m.
On 08/11/2010 04:24 AM, Holger Hans Peter Freyther wrote:
Hi all,
so the next exit command will do a double free... and
the funny part is that
the code has one assumption everything > CONFIG_NODE is considered to be
config as well. This means that we should have two enum values in libosmovty,
one for Last_Enable, one for Last_Config and have enough space between them.
And the other part... maybe vty_go_parent should return CMD_SUCCESS?
Okay, the shortest sequence to make it crash is:
enable
bts 0 oml ....
enable <- first go to parent with delete
enable <- second go to parent... double delete
we also have a similiar issue with the subscriber_put in the exit function...
the only fix I can think of is to move every node that frees data before the
CONFIG_NODE so we will not have the "auto fixup" code of VTY.
11 Aug
11 Aug
7:40 p.m.
Hi Zecke,
On Wed, Aug 11, 2010 at 04:39:44AM +0800, Holger Hans Peter Freyther wrote:
we also have a similiar issue with the subscriber_put
in the exit function...
the only fix I can think of is to move every node that frees data before the
CONFIG_NODE so we will not have the "auto fixup" code of VTY.
sounds fair to me. The auto-fixup code needs to be there for config file
parsing, where we never really know when to return from a sub-level
back to its parent, as there is no explicit 'exit' in the config file.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
7:40 p.m.
On Wed, Aug 11, 2010 at 04:24:35AM +0800, Holger Hans Peter Freyther wrote:
Hi all,
I was debugging a crash with the VTY and I am not fully understanding it but I
can easily reproduce it. The problem is triggered with the new VTY OML code
and here is what I know.
thanks for pointing this out.
1.) we move from ENABLE_NODE to the OML node. and
allocate memory
2.) when exiting the node we will free the data at vty->index.. and
set the pointer to NULL.
yes, that's what it's supposed to do.
But I can easily produce a double free issue and this
seems to be due.
strange...
cmd_execute_command:
1.) saves the vty->index to oindex and vty->node to onode
2.) tries calling vty_go_parent or such...
3.) tries more stuff...
4.) as stuff failed resets vty->index to oindex...
ah, ok. I didn't realize that try/revert nature of the vty command execution
when coming up with the application-specific go_parent() callback function.
so the next exit command will do a double free... and
the funny part is that
the code has one assumption everything > CONFIG_NODE is considered to be
config as well.
Yes, this is indeed true. I didn't really see a clean way to solve this while
generalizing the vty code into libosmovty.
This means that we should have two enum values in
libosmovty,
one for Last_Enable, one for Last_Config and have enough space between them.
that sound useful. Or we do something like |= some high bit to indicate it is
a sub-node of enable or config.
And the other part... maybe vty_go_parent should
return CMD_SUCCESS?
I don't really see anybody who uses the result of vty_go_parent(), all
the callers seem to discard the return value. However, it doesn't seem
right that vty_go_parent does not pass on the retunr value of the
host.app_info->go_parent_cb() function.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
7:50 p.m.
On 08/12/2010 01:40 AM, Harald Welte wrote:
=
the first part was wrong... changing the return will have no effect as you
have noted...
And the other
part... maybe vty_go_parent should return CMD_SUCCESS?
I don't really see anybody who uses the result of vty_go_parent(), all
the callers seem to discard the return value. However, it doesn't seem
right that vty_go_parent does not pass on the retunr value of the
host.app_info->go_parent_cb() function.
7:52 p.m.
On 08/12/2010 01:40 AM, Harald Welte wrote:
]
that sound useful. Or we do something like |= some
high bit to indicate it is
a sub-node of enable or config.
This sounds like a good idea. I will see how the number for the NODE is used
within the vty (hopefully not to determine the size of the vectors) and then
add a bit and a mask to the vty code...
5013
days inactive
5014
days old
5 comments
2 participants
participants (2)
-
Harald Welte -
Holger Hans Peter Freyther