Migrate to __system_property_read_callback() [crashpad/crashpad : main]
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
__system_property_read_callback(prop, ReadPropertyCallback, &data);Pointers to C++ types are flying across module boundaries here.
Are we guaranteed to be using the same C++ library with the same std::string implementation as this function, or whatever stored this data?
Related details
- Joshua Peraza
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Peraza (Gerrit)
Joshua Peraza voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joshua Peraza
| Commit-Queue | +1 |
1 comment
__system_property_read_callback(prop, ReadPropertyCallback, &data);Pointers to C++ types are flying across module boundaries here.
Are we guaranteed to be using the same C++ library with the same std::string implementation as this function, or whatever stored this data?
I made updates to not use std::string in the callback.
At first I didn't see the issue. Then I thought I did. Now I'm not so sure again.
The callback, called from libc.so, needed to call value->assign(). I thought this wasn't a problem because the |value| object would have a vtable pointer it uses to find the right assign() implementation. Then I thought I saw the problem was that dynamic dispatch implementation is not part of the C++ standard. However, ReadPropertyCallback() is compiled for this module so wouldn't it still use dynamic dispatch in the same way?
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
__system_property_read_callback(prop, ReadPropertyCallback, &data);Joshua PerazaPointers to C++ types are flying across module boundaries here.
Are we guaranteed to be using the same C++ library with the same std::string implementation as this function, or whatever stored this data?
I made updates to not use std::string in the callback.
At first I didn't see the issue. Then I thought I did. Now I'm not so sure again.
The callback, called from libc.so, needed to call value->assign(). I thought this wasn't a problem because the |value| object would have a vtable pointer it uses to find the right assign() implementation. Then I thought I saw the problem was that dynamic dispatch implementation is not part of the C++ standard. However, ReadPropertyCallback() is compiled for this module so wouldn't it still use dynamic dispatch in the same way?
I made updates to not use std::string in the callback.
At first I didn't see the issue. Then I thought I did. Now I'm not so sure again.
Me neither.
I thought that the `string` itself was going to be used on the other side of a module boundary. But now I think that it’s not—you’re only passing a `string*` through the module boundary, but it’s not dereferenced to a `string` until it pops out of the other side, right into the `ReadPropertyCallback` function that’s in this module.
Now I think that patch set 3 is OK.
Related details
- Joshua Peraza
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Peraza (Gerrit)
Joshua Peraza voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joshua Peraza
| Commit-Queue | +1 |
1 comment
__system_property_read_callback(prop, ReadPropertyCallback, &data);Joshua PerazaPointers to C++ types are flying across module boundaries here.
Are we guaranteed to be using the same C++ library with the same std::string implementation as this function, or whatever stored this data?
Mark MentovaiI made updates to not use std::string in the callback.
At first I didn't see the issue. Then I thought I did. Now I'm not so sure again.
The callback, called from libc.so, needed to call value->assign(). I thought this wasn't a problem because the |value| object would have a vtable pointer it uses to find the right assign() implementation. Then I thought I saw the problem was that dynamic dispatch implementation is not part of the C++ standard. However, ReadPropertyCallback() is compiled for this module so wouldn't it still use dynamic dispatch in the same way?
I made updates to not use std::string in the callback.
At first I didn't see the issue. Then I thought I did. Now I'm not so sure again.
Me neither.
I thought that the `string` itself was going to be used on the other side of a module boundary. But now I think that it’s not—you’re only passing a `string*` through the module boundary, but it’s not dereferenced to a `string` until it pops out of the other side, right into the `ReadPropertyCallback` function that’s in this module.
Now I think that patch set 3 is OK.
- Mark Mentovai
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
2 comments
data->value->assign(value);Bummed that the interface doesn’t make the length available, so you’re stuck feeling around in the dark for a NUL byte.
if (!data.read || value->empty()) {Are we sure about this? A 0-length property value should still be valid.
In the past, it wasn’t possible to distinguish between a missing property and a 0-length value when calling `__system_property_get`: both cases return 0, so it’s reasonable to consider that return value as signaling an error. But now that this has been split into separate `__system_property_find` and `__system_property_read_callback` steps, we should be able to differentiate easily.
Related details
- Joshua Peraza
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Peraza (Gerrit)
Joshua Peraza voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joshua Peraza
| Commit-Queue | +2 |
1 comment
Are we sure about this? A 0-length property value should still be valid.
In the past, it wasn’t possible to distinguish between a missing property and a 0-length value when calling `__system_property_get`: both cases return 0, so it’s reasonable to consider that return value as signaling an error. But now that this has been split into separate `__system_property_find` and `__system_property_read_callback` steps, we should be able to differentiate easily.
Yeah, an empty property value does look valid.
Related details
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
Related details
- Joshua Peraza
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Migrate to __system_property_read_callback()
__system_property_get() is deprecated. __system_property_read_callback()
replaces it as of Android API level 26.
This change was generated using gemini-cli.
- M snapshot/linux/system_snapshot_linux.cc
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |