On Thu, Jan 12, 2023 at 5:03 PM Michael Goffioul <[email protected]> wrote:
> On Wed, Jan 11, 2023 at 2:32 AM 'enh' via Android Building < > [email protected]> wrote: > >> On Tue, Jan 10, 2023 at 5:17 PM Michael Goffioul < >> [email protected]> wrote: >> >>> Well, it turns out I believe there's invalid code in the external camera >>> provider HAL, which happens to work only on 32 bits. More specifically, >>> this snippet: >>> https://cs.android.com/android/platform/superproject/+/master:hardware/interfaces/camera/provider/2.4/default/ExternalCameraProviderImpl_2_4.cpp;l=44-47 >>> >>> const char* kDevicePath = "/dev/"; >>> constexpr char kPrefix[] = "video"; >>> constexpr int kPrefixLen = sizeof(kPrefix) - 1; >>> constexpr int kDevicePrefixLen = sizeof(kDevicePath) + kPrefixLen + 1; >>> >>> The above constants are used in the code to locate the numerical value >>> in the video device name, e.g. "2" in "/dev/video2". The constant >>> kPrefixLen is computed from constant char array kPrefix, minus one, as the >>> array will have the terminating null byte (which doesn't count when >>> computing the digit offset), that is 5. But then, kDevicePrefixLen does not >>> make sense: >>> - first it uses "sizeof(kDevicePath)", which is the size of a pointer; >>> that is 4 on 32 bits, 8 on 64 bits >>> >> >> no, that's weird -- and i tell people not to do it in code review because >> it's confusing -- but it's correct because it's an _array_ rather than a >> char*. >> > > With all due respect, I think you may be mistaken here. The constant > kDevicePath is not declared as an array, but as a const char*, > oh, oops --- i may have accidentally leaked that this has been fixed internally already :-) oh, no, it's sillier than that ... there are _three_ copies of this code (default, 2.4, and 2.7), and _one_ of them -- "default", the one i looked at -- uses an array, but the other two use a pointer. > so sizeof(kDevicePath) is the size of a pointer. E.g. try to compile the > following in 32 or 64 bit: > > #include <iostream> > const char* kDevicePath_1 = "/dev/"; > constexpr char kDevicePath_2[] = "/dev/"; > int main(int argc, char** argv) { > std::cout << "sizeof(kDevicePath_1) = " << sizeof(kDevicePath_1) > << std::endl; > std::cout << "sizeof(kDevicePath_2) = " << sizeof(kDevicePath_2) > << std::endl; > return 0; > } > > I get the following results (note: I've tried with both gcc - from Fedora > 37 - and clang - from AOSP prebuilts @ android-13.0.0_r24): > > $ g++ -m32 -o testsize testsize.cpp > $ ./testsize > sizeof(kDevicePath_1) = 4 > sizeof(kDevicePath_2) = 6 > $ g++ -o testsize testsize.cpp > $ ./testsize > sizeof(kDevicePath_1) = 8 > sizeof(kDevicePath_2) = 6 > > > >> (it's actually 6 in both cases, because it also includes the '\0', which >> is one reason why people use it rather than the more obvious `strlen() + >> 1`.) >> > > But let's still assume I'm wrong, and that `sizeof(kDevicePath)` does > indeed yield 6, like you said. Then kDevicePrefixLen would be 12. However > the wanted value is the length of the string "/dev/video", that is 10. That > is, kDevicePrefixLen would have an incorrect value. > > >> what's the actual failure you're seeing? >> > > First let me stress the fact that I'm compiling the external camera > provider HAL in 64 bits mode (I'm targeting a 64-bits only build). I have 2 > USB cameras attached to the device, using /dev/video0 and /dev/video2. Both > devices are reported to cameraserver with the same identifier > [email protected]/external/0, > so basically cameraserver only sees 1 camera device. I believe that's due > this snippet in ExternalCameraProviderImpl_2_4::addExternalCamera: > > std::string cameraId = std::to_string(mCfg.cameraIdOffset + > std::atoi(devName + > kDevicePrefixLen)); > > and the fact that std::atoi returns 0 if no conversion can be made. > > As a side note, the above snippet also performs an out-of-bound array > access on 64 bits platform. Granted, the external camera service is > currently forced into 32 bits compilation in AOSP, so this out-of-bound > access is only latent. > yeah, the camera folks have filed internal bug http://b/265168485 to look at this. +Eino-Ville Talvala <[email protected]> for your extra details (and the fact that he probably already knows but was surprising to me that there are three different copies of this code!). > Michael. > > -- > -- > You received this message because you are subscribed to the "Android > Building" mailing list. > To post to this group, send email to [email protected] > To unsubscribe from this group, send email to > [email protected] > For more options, visit this group at > http://groups.google.com/group/android-building?hl=en > > --- > You received this message because you are subscribed to the Google Groups > "Android Building" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/android-building/CAB-99LvLjaNO3yFP%3Dt7MuP6BAo71UqyEictSpCdVWWOXNZvhyA%40mail.gmail.com > <https://groups.google.com/d/msgid/android-building/CAB-99LvLjaNO3yFP%3Dt7MuP6BAo71UqyEictSpCdVWWOXNZvhyA%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- -- You received this message because you are subscribed to the "Android Building" mailing list. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/android-building?hl=en --- You received this message because you are subscribed to the Google Groups "Android Building" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/android-building/CAJgzZoo2DoSLQW-RBat7rPJtEpOBc8fqoWKduzQuARQmA9oUwg%40mail.gmail.com.
