CDataFinalizer should be able to maintain the exact type of a CData

RESOLVED FIXED in mozilla15

Status

()

Core
js-ctypes
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

At the moment, if we have a CData holding a |char*| and we pass it to a CDataFinalizer with finalizer |free|, the CDataFinalizer will itself be listed as |void*|, because we are only looking at the type of the function.

In this case, we need to be able to return |char*|.
Created attachment 618262 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct
Assignee: nobody → dteller
Status: NEW → ASSIGNED
Attachment #618262 - Flags: review?(jorendorff)
Created attachment 618263 [details] [diff] [review]
Returning the result of dispose

Same one, but a non-empty patch.
Attachment #618262 - Attachment is obsolete: true
Attachment #618262 - Flags: review?(jorendorff)
Attachment #618263 - Flags: review?(jorendorff)
Created attachment 618269 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Fixing the tests. Sorry about the snafu. Should now be ok.
Attachment #618263 - Attachment is obsolete: true
Attachment #618263 - Flags: review?(jorendorff)
Attachment #618269 - Flags: review?(jorendorff)
Blocks: 748745
Depends on: 745184
Comment on attachment 618269 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

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

Wow. The fact that it's like this is actually a kind of interesting observation on its own. It kind of breaks how I was thinking of CDataFinalizer (you learn something every day). What kind of real-world use cases are we addressing here? Are there also use cases where ImplicitConvert is too strict?

We probably want a few more tests that this isn't going to go horribly wrong. For example, what if the type of the CData object is a smaller size than the finalizer argument type? I think you could get valgrind errors or correctness errors, maybe crashes. Note that CDataFinalizer::Construct allocates a buffer of type objArgType, but CDataFinalizer::Methods::Forget assumes that the buffer holds a value of exactly GetCType(obj).

I think if we need this, CDataFinalizer::Construct should be pretty strict about it. Perhaps it should allow this only in cases where the argument type and the CData type are both pointer types of the same size. Other cases should throw--which should be easy enough to test.

Clearing r?.
Attachment #618269 - Flags: review?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Comment on attachment 618269 [details] [diff] [review]
> Recording a more precise type in CDataFinalizer::Construct
> 
> Review of attachment 618269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow. The fact that it's like this is actually a kind of interesting
> observation on its own. It kind of breaks how I was thinking of
> CDataFinalizer (you learn something every day). What kind of real-world use
> cases are we addressing here? Are there also use cases where ImplicitConvert
> is too strict?

The specific scenario is the following:
- I have obtained a |char*| from some C function (in that case, |getwd(NULL)|);
- I need to deallocate that |char*| with |free|, with a CDataFinalizer;
- I also need to pass that |char*| to |readString| and other functions that take a |char*|.
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> We probably want a few more tests that this isn't going to go horribly
> wrong. For example, what if the type of the CData object is a smaller size
> than the finalizer argument type? I think you could get valgrind errors or
> correctness errors, maybe crashes. Note that CDataFinalizer::Construct
> allocates a buffer of type objArgType, but CDataFinalizer::Methods::Forget
> assumes that the buffer holds a value of exactly GetCType(obj).
> 
> I think if we need this, CDataFinalizer::Construct should be pretty strict
> about it. Perhaps it should allow this only in cases where the argument type
> and the CData type are both pointer types of the same size. Other cases
> should throw--which should be easy enough to test.

I assumed (wrongly, I guess) that the call to |ImplicitConvert| would check that nothing could go wrong at http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#6585

Now, as you suggest, I could easily add a safety net when I obtain |objBestArgType|, by checking that |objBestArgType| and |objArgType| have the same size and are pointers. Is there a way of structurally comparing two CType to ensure that I only check that |objBestArgType| is a pointer only if it is not equal to |objArgType|?
Created attachment 621522 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Added more careful checks. Do you think that this is sufficient?
Attachment #618269 - Attachment is obsolete: true
Attachment #621522 - Flags: review?(jorendorff)
Comment on attachment 621522 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

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

r=me.

You might consider adding a test for a case where the type of the CData object is clearly wrong. It should throw TypeError.

::: js/src/ctypes/CTypes.cpp
@@ +6629,5 @@
> +      objBestArgType = CData::GetCType(objData);
> +      size_t sizeBestArg;
> +      if (!CType::GetSafeSize(objBestArgType, &sizeBestArg)) {
> +        return TypeError(cx, "(an object with known size)", valData);
> +      }

You can add a MOZ_UNREACHED here. We only create CData objects of types that have a size.

The only types without sizes are void (for use as a return type) and array types with unspecified length (for use as argument types); there are no CData objects of either type.
Attachment #621522 - Flags: review?(jorendorff) → review+
Created attachment 621623 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Added a test, replaced exception with assertion. Thanks for the quick review!
Attachment #621522 - Attachment is obsolete: true
Created attachment 622315 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct
Attachment #621623 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d04efcf7237e
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/d04efcf7237e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.