In a console program I can't reproduce this problem, either with at() or with &iter->second. (I added logging to the destructor just to be sure) It may be some other UB in my program that's somehow trashing the heap in production. Sorry for the bother.
Thanks! Mohamed On Wed, Feb 13, 2019 at 5:57 PM Mohamed Koubaa <[email protected]> wrote: > Adam, > > The point about the style guide is well-taken, I also understood that this > is a deliberate divergence from the contract of std::map. > > > The function signature of at() claims it returns a reference but in > practice it->second (at least in v3.2) returns by value. So I think the > method may be depending on UB but like I said I don't claim to understand > this completely. What I can report is that I observed a crash in my use > case with a use-after free from doing a find and returning &iter->second as > you suggest. I'll add that my code isn't quite as simple as > TryGetValue(key)->field(); in that I pass the pointer along to other code > when the stack probably looks different. > > Thanks, > Mohamed > > On Wed, Feb 13, 2019 at 5:24 PM Adam Cozzette <[email protected]> > wrote: > >> Google's C++ style avoids the use of exceptions, so I think that explains >> why it does a GOOGLE_CHECK instead of throwing an exception. About your >> use-after-free concern, I haven't read through too closely but I believe >> everything is fine there because at() is returning a reference and not a >> copy. You could probably simplify your TryGetValue() function by having it >> do just a single lookup: my_map.find(key) returns an iterator so once >> you've verified that it's not equal to end() you can return &iter->second. >> >> On Wed, Feb 13, 2019 at 12:19 PM Mohamed Koubaa <[email protected]> >> wrote: >> >>> Hello, >>> >>> I'm using version 3.2. I have a google::protobuf::map<int, Msg> and >>> would like to implement a function that returns either a pointer to value >>> in the map given or a key or null if it does not exist. If I was using a >>> std::map, I would do this. >>> >>> Msg const * Foo::TryGetValue(int key) const { >>> try { >>> auto& value = my_map.at(key); >>> return &value; >>> } catch (...) { >>> return nullptr; >>> } >>> } >>> >>> Because protobuf's contract for "at" is different (it does a >>> GOOGLE_CHECK rather than a throw if the item with the given key is >>> different), I thought I could implement something similar like this, with >>> the downside of doing two lookups rather than one. >>> >>> Msg const * Foo::TryGetValue(int key) const { >>> auto iter = my_map.find(key); >>> if (iter == my_map.end()) >>> return nullptr; >>> auto& value = my_map.at(key); >>> return &value; >>> } >>> >>> However, after peeking at the implementation of google::protobuf::map >>> (and I don't claim to understand it completely) it seems that the "at" >>> method does a find and then just returns iter->second which does a copy. >>> So the client of TryGetValue will have used after free. Is there another >>> way to do this? >>> >>> Thanks! >>> Mohamed Koubaa >>> Software Developer >>> Ansys, Inc >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Protocol Buffers" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> To post to this group, send email to [email protected]. >>> Visit this group at https://groups.google.com/group/protobuf. >>> For more options, visit https://groups.google.com/d/optout. >>> >> -- You received this message because you are subscribed to the Google Groups "Protocol Buffers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at https://groups.google.com/group/protobuf. For more options, visit https://groups.google.com/d/optout.
