argdata.cpp: Cannot assign map to variable and use it later
Closed this issue · 9 comments
The following segfaults. The first loop parses the top-level argdata tuple, and stores the values in local variables. Later on these are processed. But for the map it doesn't work.
argdata_t::map args;
for (auto i: ad->as_map()) {
auto key = i.first->as_str();
if(key == "args") {
args = i.second->as_map();
}
}
/* later on */
for (const auto &entry: args) {
/* do something with entry.first and entry.second */
}Is this intentional?
I suppose it holds a pointer to a temporary object that goes out of scope.
Is it somehow possible to hold a reference/pointer to something deeper in the argdata?
Iterating over a seq or map will decode serialized data on the fly (unless it's an argdata_t created by create_map/seq, which doesn't contain serialized_data). The decoded argdata_t is only kept temporarily in the iterator, and is gone when going to the next element. argdata_t::map only contains a reference to the argdata_t of the map. So indeed, this doesn't work. I'll document this better.
Keeping (a copy) of the iterator around to the element of the outer map instead of an argdata_t::map should work, but doesn't look very nice.
We're discussing possible solutions in #cloudabi right now. We'll probably change the library a bit to make things like this easier.
Maurice and I just discussed this in more detail and the resolution will likely be as follows: argdata_t::map shouldn't hold a reference to the argdata_t from which it was instantiated, as it can be implemented in such a way that it doesn't need to. Fixing that should make the code you presented above work.
So far I haven't run into issues like these when using the C API, where the scoping is a bit more sloppy. It's good to get stuff like this tackled!
So here's the solution we came up with: We'll make the iterator (and argdata_t::map) contain pointers to the underlying data (the serialized buffer, or the key and value arrays), instead of a pointer to the argdata_t containing those pointers. Then only the underlying data needs to stay alive, instead of the (temporary) argdata_t as well. (There was no need for this extra level of indirection here, and removing it fixes this issue.)
Working on that right now.
Thanks for the fast response. Yes, that sounds like a good solution!
@laanwj The code snippet you posted should work with the new version. (It does in my test.) Please verify.
@m-ou-se I suspect @laanwj is testing this on CloudABI itself, meaning he has to replace the copy of argdata that has been imported into cloudlibc.
As the cloudlibc unit tests still pass with change applied, I've just merged in argdata-0.2, tagged cloudlibc-0.72 and fired up a package build. @laanwj: You should be able to pkg upgrade -r CloudABI tomorrow morning to get this fixed!
New packages are live!
I've updated the packages and can confirm that this works now.
Great, thanks for the quick fix!
Closing this issue.