Shouldn't memset structs containing std::string
In the new C++ midas, several structs now have std::string members (e.g. callback_addr
and RPC_CLIENT_CONNECTION
in msystem.h). Unfortunately, there are still places that try to use memset to initialise/reset these structures. This is undefined behaviour, and leads to crashes in my system.
Example problematic calls are:
- mserver.cxx:306 - on my system spawned mserver processes segfault when trying to write to
callback.host_name
on L322, and we cannot run any remote frontends. - midas.cxx:10987 - on my system clients segfault when later trying to destruct the client connection object.
There are at least half a dozen other instances just for the structs in msystem.h.
I think we should define a common pattern for resetting such structures, and apply it consistently. A couple of immediately-obvious options are:
- Define a
reset()
function in each struct. We would have to remember to update it whenever the struct layout changes. - Change them to nested structs, so the POD types are in an inner struct which can be memset. We would probably still need a
reset()
function to reset the strings to""
, so I prefer the first option over this one.
There may be cleaner / simpler solutions I’m missing. Thoughts?
Comments (2)
-
-
reporter - changed status to resolved
This is now fixed in 9241f17 by implementing
clear()
for the relevant structs in msystem.h. There were also some places that used memcpy which I replaced with a call to the default copy-constructor.We should be sure to check for memset/memcpy usage for any more structs that are converted to strings in future.
- Log in to comment
I agree we should go with a reset() function for each structure. Or call it clear().