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

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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*|.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recording a more precise type in CDataFinalizer::Construct

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Returning the result of dispose

Same one, but a non-empty patch.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recording a more precise type in CDataFinalizer::Construct

Fixing the tests. Sorry about the snafu. Should now be ok.

Comment 4

[Jason Orendorff [:jorendorff]]

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?.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

(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*|.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(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|?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recording a more precise type in CDataFinalizer::Construct

Added more careful checks. Do you think that this is sufficient?

Comment 8

[Jason Orendorff [: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.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recording a more precise type in CDataFinalizer::Construct

Added a test, replaced exception with assertion. Thanks for the quick review!

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recording a more precise type in CDataFinalizer::Construct

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d04efcf7237e

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d04efcf7237e