1004434 - MaxFileSize, MaxVideoLength of CameraControl Interface recording options should be of 64 bits

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[vasanth]

Follow up of bug 996560 comment 21 

That bug's fix fails when >4GB remaining space (value more than 32 bits) is available in sdcard.

CameraStartRecordingOptions.maxFileSizeBytes is long long [1]
whereas StartRecordingOptions.maxFileSizeBytes is just uint32 [2]

Hence in this assignment, [3] only lower 32 bits of maxFileSizeBytes are assigned which is wrong.

Will change StartRecordingOptions.maxFileSizeBytes type to uint64_t and upload here.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/CameraControl.webidl#95
[2] http://dxr.mozilla.org/mozilla-central/source/dom/camera/ICameraControl.h#113
[3] http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#765

Comment 1

[vasanth]

Attachment: Bug1004434-MaxFileSize-MaxVideoLength-Camera-Recording-Options-64bit.patch

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8416325 [details] [diff] [review]
Bug1004434-MaxFileSize-MaxVideoLength-Camera-Recording-Options-64bit.patch

Review of attachment 8416325 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, but it exposes a problem with nsGonkCameraControl::SetupRecording(), which currently takes signed integers:
http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#1540

Please fix this as well, and add a range check to SetupRecording() for file size and video length before setting the recorder properties; something like:
  if (x > INT64_MAX) {
    DOM_CAMERA_LOGE("max...=%llu is too big\n", x);
    return NS_ERROR_INVALID_ARG;
  }

Comment 3

[vasanth]

(In reply to Mike Habicher [:mikeh] from comment #2)
> This is a good start, but it exposes a problem with
> nsGonkCameraControl::SetupRecording(), which currently takes signed integers:
> http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.
> cpp#1540

I see later anyways GonkRecorder::SetParamMaxFileSizeBytes() clips this value to a max of 4GB [1]. 
Not sure an additional check is needed? 
We can just use int64_t instead of uint64_t for this params if you would like?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkRecorder.cpp#374

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to vasanth from comment #3)
> (In reply to Mike Habicher [:mikeh] from comment #2)
> > This is a good start, but it exposes a problem with
> > nsGonkCameraControl::SetupRecording(), which currently takes signed integers:
> > http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.
> > cpp#1540
> 
> I see later anyways GonkRecorder::SetParamMaxFileSizeBytes() clips this
> value to a max of 4GB [1]. 
> Not sure an additional check is needed? 

Okay, that check is fine.

> We can just use int64_t instead of uint64_t for this params if you would like?

I don't think it makes sense to allow file sizes to be negative. I think the correct solution is to change maxFileSizeBytes and maxVideoLengthMs in CameraControl.webidl[2] to "unsigned long long" and making sure everything that calls down to GonkRecorder::SetParamMaxFileSizeBytes() consistently uses uint64_t. We can leave the final cast/ceiling operation to that function.

> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkRecorder.cpp#374

2. http://dxr.mozilla.org/mozilla-central/source/dom/webidl/CameraControl.webidl#95

Comment 5

[vasanth]

(In reply to Mike Habicher [:mikeh] from comment #4)
> I don't think it makes sense to allow file sizes to be negative. I think the
> correct solution is to change maxFileSizeBytes and maxVideoLengthMs in
> CameraControl.webidl[2] to "unsigned long long" and making sure everything that calls down to 
> GonkRecorder::SetParamMaxFileSizeBytes() consistently uses uint64_t.

I agree that file size can't be -ve. GonkRecorder::SetParamMaxFileSizeBytes() is called by GonkRecorder::SetParameter() which uses only signed (32/64) integers for all params. [1] 
So just modifying max file size to uint64_t won't be consistent in this way? 
In this case, only size matters here and not the type. 
So shall we go ahead with the existing patch?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkRecorder.cpp#513

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to vasanth from comment #5)
> 
> I agree that file size can't be -ve.
> GonkRecorder::SetParamMaxFileSizeBytes() is called by
> GonkRecorder::SetParameter() which uses only signed (32/64) integers for all
> params. [1] 
> So just modifying max file size to uint64_t won't be consistent in this way? 
> In this case, only size matters here and not the type. 
> So shall we go ahead with the existing patch?

I've just looked at the code, and it's worse than that. GonkRecorder::setParameter() is broken. Although it uses strtoll() to parse the string argument, which checks that the argument will fit into a |long long|, in the case of video time limit, the code then blindly multiplies that value by 1000LL, which can overflow the int64_t into something much smaller.

e.g. maxVideoLengthMs = 0x2000000000000067 * 1000 = 0x19258, a synthetic case, but entirely wrong behaviour.

Either we need to fix GonkRecorder::SetParameter(), or make sure nsGonkCameraControl::SetupRecording() is smart enough to prevent this.

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Make video limits use uint64_t, with sanity checks, v1

A patch that uses unsigned 64-bit integers, and sanity checks them for the GonkRecorder.

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

This update adds [Clamp] to the dictionary members, so that values outside the range of the 'unsigned long long' are clamped to 0..ULLONG_MAX.

Comment 9

[vasanth]

Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

Verified it works. Doesn't apply on V1.4, please upload the rebased patch for V1.4 as well

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Make video limits use uint64_t, with sanity checks [b2g30_v1_4] v2

Rebased for b2g30_v1_4.

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

bz, would you mind looking over the WebIDL changes (again)?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

r=me

Comment 13

[Preeti Raghunath(:Preeti)]

Hema

Can someone else review for Dave. Looks like he is away too.

Comment 14

[Hema Koka [:hema]]

Sotaro,

I think Mike probably already reached out to you on Friday for review before he left on vacation. Could you please help here?

Thanks
Hema

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

Review of attachment 8418956 [details] [diff] [review]:
-----------------------------------------------------------------

Just a small nit, otherwise this looks good to me.

::: dom/camera/GonkCameraControl.cpp
@@ +1557,3 @@
>  
> +  DOM_CAMERA_LOGI("maxVideoLengthMs=%llu\n", aMaxVideoLengthMs);
> +  const int64_t kMaxVideoLengthLimit = INT64_MAX / 1000;

nit: Rather than Length, I think that using Millisecs (or microsecs) would be more useful (Length doesn't give any sense of units).

At the very least a comment would be nice.

nit: kMaxVideoLengthLimit should be a uint64_t so that we don't get warnings about mixing signed/unsigned types.

Comment 16

[Hema Koka [:hema]]

Thanks Dave!

Comment 17

[Inder]

Hema -- considering MikeH is on PTO who is addressing the nit Dave highlighted? Do you want Vasanth to take care of it?

Comment 18

[Dave Hylands [:dhylands]]

I think it would be fine to address the nits in a followup bug. I guess the other question to ask is who will be landing this?

Comment 19

[Preeti Raghunath(:Preeti)]

Taking Hema's ni:

Dave

Can you please land this? Not sure if partner should.

Comment 20

[Dave Hylands [:dhylands]]

Attachment: Make video limits use uint64_t, with sanity checks [b2g-inbound] v3

Fixed nits

Comment 21

[Dave Hylands [:dhylands]]

Attachment: Make video limits use uint64_t, with sanity checks [b2g-inbound] v3

Added proper r=

Comment 22

[Dave Hylands [:dhylands]]

Attachment: Make video limits use uint64_t, with sanity checks [b2g30_v1_4] v3

Updated with nits

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9619b6278c49

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9619b6278c49

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/364a02a3fd9c

Comment 26

[[:RJ]]

No STR is present to create test case to address bug.