Re: External camera service for 64 bits

189 views
Skip to first unread message

Michael Goffioul

unread,
Jan 10, 2023, 8:17:20 PM1/10/23
to android-...@googlegroups.com
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
- second it adds one to the invalidly computed length of "/dev/video" (I'm assuming that was the intent of the author)
Assuming the author thought "sizeof(kDevicePath)" would yield the length of the constant string "/dev/" (I think that a reasonable assumption), then incrementing by one "sizeof(kDevicePath) + kPrefixLen" does not make sense, as the non-incremented value is what you're after. But on 32 bits, something funny happens: the length of "/dev/" is 5, and sizeof(kDevicePath) is 4. Maybe the author noticed that kDevicePrefixLen didn't have the expected value, then noticed the previous line where kPrefixLen is decremented by one, thought kPrefixLen should then be re-incremented, and Bob's your uncle. Works fine on 32 bits, falls flat on 64 bits.

Not sure if anybody at Google is reading this, but you might wanna have a look at that more closely.

Michael.




On Thu, Sep 29, 2022 at 3:09 PM Michael Goffioul <michael....@gmail.com> wrote:
Is there a particular reason to keep the external camera service a 32-bit only component [1]? From what I can see, the cameraserver process has been made available for 64-bit build [2].

Thanks,

enh

unread,
Jan 10, 2023, 8:32:12 PM1/10/23
to android-...@googlegroups.com
On Tue, Jan 10, 2023 at 5:17 PM Michael Goffioul <michael....@gmail.com> 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*. (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`.)
 
- second it adds one to the invalidly computed length of "/dev/video" (I'm assuming that was the intent of the author)
Assuming the author thought "sizeof(kDevicePath)" would yield the length of the constant string "/dev/" (I think that a reasonable assumption), then incrementing by one "sizeof(kDevicePath) + kPrefixLen" does not make sense, as the non-incremented value is what you're after. But on 32 bits, something funny happens: the length of "/dev/" is 5, and sizeof(kDevicePath) is 4. Maybe the author noticed that kDevicePrefixLen didn't have the expected value, then noticed the previous line where kPrefixLen is decremented by one, thought kPrefixLen should then be re-incremented, and Bob's your uncle. Works fine on 32 bits, falls flat on 64 bits.

what's the actual failure you're seeing?
 
Not sure if anybody at Google is reading this, but you might wanna have a look at that more closely.

Michael.




On Thu, Sep 29, 2022 at 3:09 PM Michael Goffioul <michael....@gmail.com> wrote:
Is there a particular reason to keep the external camera service a 32-bit only component [1]? From what I can see, the cameraserver process has been made available for 64-bit build [2].

Thanks,

--
--
You received this message because you are subscribed to the "Android Building" mailing list.
To post to this group, send email to android-...@googlegroups.com
To unsubscribe from this group, send email to
android-buildi...@googlegroups.com
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 android-buildi...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/android-building/CAB-99Lt_g9A_vA0jrY4fb-ACJH-sWWQZJwEny_AmCSwPjtHmjQ%40mail.gmail.com.

Michael Goffioul

unread,
Jan 12, 2023, 8:03:29 PM1/12/23
to android-...@googlegroups.com
On Wed, Jan 11, 2023 at 2:32 AM 'enh' via Android Building <android-...@googlegroups.com> wrote:
On Tue, Jan 10, 2023 at 5:17 PM Michael Goffioul <michael....@gmail.com> 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*, 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 device@3.4/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.

Michael.

enh

unread,
Jan 12, 2023, 8:09:40 PM1/12/23
to android-...@googlegroups.com, Eino-Ville Talvala
On Thu, Jan 12, 2023 at 5:03 PM Michael Goffioul <michael....@gmail.com> wrote:
On Wed, Jan 11, 2023 at 2:32 AM 'enh' via Android Building <android-...@googlegroups.com> wrote:
On Tue, Jan 10, 2023 at 5:17 PM Michael Goffioul <michael....@gmail.com> 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.
yeah, the camera folks have filed internal bug http://b/265168485 to look at this. +Eino-Ville Talvala 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 android-...@googlegroups.com
To unsubscribe from this group, send email to
android-buildi...@googlegroups.com
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 android-buildi...@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages