Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

php_com_dotnet - can not create VT_ERROR variant type #8750

Closed
systoolz opened this issue Jun 10, 2022 · 8 comments
Closed

php_com_dotnet - can not create VT_ERROR variant type #8750

systoolz opened this issue Jun 10, 2022 · 8 comments

Comments

@systoolz
Copy link

Description

Can not create VT_ERROR variant type. It's required for some COM method calls where you need to specify an empty argument, since it's VT_ERROR type variant with error DISP_E_PARAMNOTFOUND (0x80020004) as actual error code (see here).

The following code:

<?php // database.php
  // next line works only in PHP 4.4.9 (PHP 5, 6, 7 and 8 are bugged)
  $vtMissing = new VARIANT(0x80020004, VT_ERROR);
  $db = new COM('ADODB.Connection');
  $db->ConnectionString = 'Provider=Microsoft.Jet.OLEDB.4.0;Data Source=database.mdb';
  $db->Mode = 1; // adModeRead
  $db->Open();
  // adSchemaProviderSpecific, *missing*, JET_SCHEMA_USERROSTER
  $rs = $db->OpenSchema(-1, $vtMissing, '{947bb102-5d43-11d1-bdbf-00c04fb92675}');
  // manual counting since rs.RecordCount is -1 (not supported)
  $i = 0;
  while (!$rs->EOF) {
    $rs->MoveNext();
    $i++;
  }
  $rs->Close();
  $rs = null;
  $db->Close();
  $db = null;

  echo $i;

Resulted in this output:

Fatal error: Uncaught com_exception: Variant type conversion failed: Type mismatch in Z:\database.php:3
Stack trace:
#0 Z:\database.php(3): variant->__construct(2147614724, 10)
#1 {main}
  thrown in Z:\database.php on line 3

But I expected this output instead:

1

Test file "database.mdb" can be any Microsoft Access database file, even without any tables inside.
More details can be found in this article (auto-translated from Russian):
VT_MISSING argument in PHP for COM objects

PHP Version

PHP 8.1.7 x64

Operating System

Windows 7 x64

@cmb69
Copy link
Member

cmb69 commented Jun 12, 2022

Thanks for reporting, and the good research!

Can not create VT_ERROR variant type.

Yeah. That is because as of PHP 5.0.0 the algorithm first creates a variant that matches the given $value without even considering the $type, and then tries to convert it to that $type. This doesn't work for VT_ERROR (and relatedly to typed arrays).

While it certainly would be possible to allow to create VT_ERROR variants, I don't think this makes much sense generally. As I understand it, your case (VT_ERROR type variant with error DISP_E_PARAMNOTFOUND) is the only one that needs to be supported. As such, another way to get that variant appears to make more sense. Maybe in form of a special function, or a creation method on variant, or maybe even a constant.

However, PHP still doesn't allow to skip parameters, but has introduced named parameters, so using these might be an option. I gave that a quick try, but that leads to a crash in the following code:

php-src/Zend/zend_execute.c

Lines 4747 to 4748 in 2dc9026

zend_internal_arg_info *arg_info = &fbc->internal_function.arg_info[i];
size_t len = strlen(arg_info->name);

I don't think this is com_dotnet specific, but likely rather happens for all internal objects which allow dynamic methods (i.e. basically implement ::__call()). I think that can be fixed, and named arguments could be supported by com_dotnet, but that would be a much bigger task than offering a way to get a VT_ERROR type variant with error DISP_E_PARAMNOTFOUND which could be passed explicitly by the programmer.

@systoolz
Copy link
Author

Thank you for detailed in-depth comment!
If this is the only case where VT_ERROR type is required, then I totally agree with you and it would be great to have something that could be used as a valid missing argument, be it a constant, a special function, or anything else.
Thank you!

cmb69 added a commit to cmb69/php-src that referenced this issue Jun 21, 2022
Despite the current documentation claiming that `variant::__construct()`
would allow to create `VT_ERROR` variants, that is not possible.  It is
not even clear whether this would be desirable, so we introduce
`variant::createError()` which explicitly allows to do this.

We also introduce `DISP_E_PARAMNOTFOUND` which might be the most
important `scode` for this purpose, since this allows to skip optional
parameters in method calls.
@cmb69
Copy link
Member

cmb69 commented Jun 21, 2022

I had a further look at this, and have to admit that the constructor not working for VT_ERROR is really strange, especially that it is documented:

Possible values are one of the VT_XXX Predefined Constants.

And VT_ERROR is clearly listed there. Supporting VT_ERROR there would be trivial, but it would likely be confusing if someone wanted to convert a VT_ERROR variant to a string; should that print the scode, or fail, or whatever. Since the round-trip is apparently not well defined, I think we should indeed not support to create VT_ERROR variant via variant::__construct(), but instead fix the documentation.

As I understand it, your case (VT_ERROR type variant with error DISP_E_PARAMNOTFOUND) is the only one that needs to be supported.

I'm no longer sure about that. There may be other use cases where creating VT_ERROR variants would be necessary, and I don't see the particular need to only support DISP_E_PARAMNOTFOUND.

@cmb69 cmb69 linked a pull request Jun 21, 2022 that will close this issue
@systoolz
Copy link
Author

As I see variant::createError() accepts only integers as values. Is it possible in that case to treat VT_ERROR as VT_INT internally when creating VT_ERROR? I.e. limit scode values only to integers? This will solve next things:

  • converting VT_ERROR to string same as VT_INT to string (throw an error if variant type VT_ERROR for example returned from COM object and didn't contain integer type in scode), I don't think it's worth to support DISP_E_* string constants since in PHP it's limited and people always can find error code names by actual integer values
  • it's still new variant(DISP_E_PARAMNOTFOUND, VT_ERROR) as it specified in documentation, i.e. people will not be confused with two ways of creating VT_ERROR type: new one with variant::createError() and old one with new variant() and VT_ERROR which still there and still throws an error
  • throw Variant type conversion failed: Type mismatch as it was now when creating VT_ERROR for any other types except integer as scode value

@cmb69
Copy link
Member

cmb69 commented Jun 26, 2022

First, an scode is always an integer, namely a unsigned 32bit integer. That is the reason why I suggest to add the DISP_E_PARAMNOTFOUND constant, because on 32bit systems, its value would have to be negative, since 0x80020004 would be converted to float by PHP.

It is certainly debatable whether it is better to provide a new method (::createError()) or to support creating VT_ERROR via the constructor. I don't think having both makes much sense. However, I went with the new method, because I'm strictly opposed to have implicit conversion the other way round (i.e. convert variant(VT_ERROR) to an integer zval). This could easily lead to hard to find bugs, e.g. when doing artihmetic on a function result which is supposed to some integer, but actually is a VT_ERROR. It should be noted that VBScript does not allow any creation or manipulation of error variants, and although VB allows creation and explicit conversion of error variants, it still does not support implicit conversion.

We could support explicit conversion from error variants to integer variants via variant_cast(); if we consider that useful, I think it would make sense to support variant::__construct(…, VT_ERROR) instead of introducing the new method variant::createError().

@systoolz
Copy link
Author

Thank you for providing more details!
I fully agree with all of your points.
I have to say that I can only suggest something or give feedback from the end user's point of view. I don't know if implementing certain things would be time consuming or not useful at all. So please consider my comments as humble suggestions only.
Thank you for your help!

cmb69 added a commit to cmb69/php-src that referenced this issue Jun 28, 2022
We add support for creating `VT_ERROR` variants via `__construct()`,
and allow casting to int via `variant_cast()` and `variant_set_type()`.
We do not, however, allow type conversion by other means, to avoid
otherwise easily introduced type confusion.  VB(A) also only allows
explicit type conversion.

We also introduce `DISP_E_PARAMNOTFOUND` which might be the most
important `scode` for this purpose, since this allows to skip optional
parameters in method calls.
@cmb69 cmb69 linked a pull request Jun 28, 2022 that will close this issue
@cmb69
Copy link
Member

cmb69 commented Jun 28, 2022

I just submitted PR #8886 which would be an alternative implementation.

@systoolz
Copy link
Author

Thank you!
In my humble opinion, the alternative implementation in PR #8886 is exactly what is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants