#322 ByRef 2D arrays of 1 Byte improperly handled

[wyrock]

If a COM module passes a 2D array where one each dimension is only one byte, then win32com only returns a buffer. The buffer prevents access from the other dimension of the array. It is expected to get an array of buffers.

The bug lies in oleargs.cpp. I've attached an updated version of oleargs.cpp and also some modified versions of the com_testing code to show the problem.

Could someone please review the proposed fix and commit the change?

Thanks,

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

File Added: com_testing2.zip

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

File Added: testSmaller.py

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

File Added: oleargs.new.cpp

[Mark Hammond]

Logged In: YES
user_id=14198
Originator: NO

Thanks! I haven't looked yet, but I can believe what you say is true. I'll (try to) get to this before the next release (but a .diff against the test suite instead of the .zip with a complete copy would help even more!) If you have the c compiler handy, then 'setup.py install' from CVS is quite an easy way to get a build from sources (but I digress... :)

[wyrock]

CVS diff showing the code changes used

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

Attaching the cvs diff with the fix (in oleargs.cpp) and some modifications to the test bench that test for the issue.
File Added: diff.out

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

File Added: test2dByRef.py

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

Now that I've built the source with the fix. I know i can do setup.py -q install, but is there an existing script for making the win32 .exe that gets distributed?

Also, the diff is fairly short, so I'll just post directly here:

? diff.out
? com/TestSources/PyCOMTest/PyCOMTest.plg
? com/win32com/src/oleargs.orig.cpp
? win32/src/_winxpthememodule.backup.cpp
Index: com/TestSources/PyCOMTest/PyCOMImpl.cpp
===================================================================
RCS file: /cvsroot/pywin32/pywin32/com/TestSources/PyCOMTest/PyCOMImpl.cpp,v
retrieving revision 1.15
diff -r1.15 PyCOMImpl.cpp
312a313,347
> STDMETHODIMP CPyCOMTest::GetComplexVariantByRef(VARIANT *out)
> {
> VariantClear(out); // necessary?
> // Creating a safe array with two dimensions to make sure
> // an array of buffers can be returned
> SAFEARRAYBOUND sabBound[2]={ {5,0}, {8,0}};
> SAFEARRAY *psaDataBuffer = SafeArrayCreate(VT_UI1,2,sabBound);
> VariantInit(out);
> out->vt = VT_ARRAY | VT_UI1;
> out->parray = psaDataBuffer;
> long indicies[2] = {0,0};
> for(int i=0;i<5;i++){
> indicies[0]=i;
> for(int j=0;j<8;j++){
> indicies[1] = j;
> SafeArrayPutElement(psaDataBuffer,indicies,&j);
>
> }
> }
> return S_OK;
> }
>
>
> STDMETHODIMP CPyCOMTest::SetComplexVariantByRef(VARIANT *v)
> {
>
> if( v->parray->cDims == 2 && v->parray->rgsabound[1].cElements==5 &&
> v->parray->rgsabound[0].cElements==8){
> return S_OK;
> }
> return -1;
> }
>
>
>
Index: com/TestSources/PyCOMTest/PyCOMImpl.h
===================================================================
RCS file: /cvsroot/pywin32/pywin32/com/TestSources/PyCOMTest/PyCOMImpl.h,v
retrieving revision 1.14
diff -r1.14 PyCOMImpl.h
81a82,84
> STDMETHOD(GetComplexVariantByRef)(VARIANT *out);
> STDMETHOD(SetComplexVariantByRef)(VARIANT *v);
>
Index: com/TestSources/PyCOMTest/PyCOMTest.dsp
===================================================================
RCS file: /cvsroot/pywin32/pywin32/com/TestSources/PyCOMTest/PyCOMTest.idl,v
retrieving revision 1.15
diff -r1.15 PyCOMTest.idl
198a199,200
> HRESULT GetComplexVariantByRef([out, retval] VARIANT *vout);
> HRESULT SetComplexVariantByRef([out] VARIANT *v);
Index: com/win32com/src/oleargs.cpp
===================================================================
RCS file: /cvsroot/pywin32/pywin32/com/win32com/src/oleargs.cpp,v
retrieving revision 1.37
diff -r1.37 oleargs.cpp
398c398
< if (dimNo==nDims && vt==VT_UI1 && obj->ob_type->tp_as_buffer) {
---
> if (dimNo==nDims && obj->ob_type->tp_as_buffer) {
831c831
< if (vt==VT_UI1) {
---
> if (vt==VT_UI1 && dimNo==nDims) {

[Mark Hammond]

Logged In: YES
user_id=14198
Originator: NO

Thanks - I'll look at this when I get time. You can build an executable by running:

setup.py bdist_wininst

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

Mark,

I was just curious if you had been able to review this change and add it to the current source tree?

Thanks,
Kevin

[Mark Hammond]

Logged In: YES
user_id=14198
Originator: NO

I'm adding a new patch that doesn't attempt to return multiple buffers - I couldn't make the old patch return the correct data, and http://mail.python.org/pipermail/python-win32/2007-July/006096.html convinces me that the right thing to do may well be to return a single buffer in this case, so long as the caller can get the dimensions - that way, if the data is destined for PIL etc, its very efficient, but still simple for someone to slice up if they really do need it as a multi-dim array.

The patch works, but I'm reluctant to check it in until we do return the dimensions. This might mean creating a new buffer-like object.
File Added: 2d_ui1.patch

[Mark Hammond]

patch including tests

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

I'm sorry the patch I submitted didn't work. Not sure what went wrong there.

The patch you proposed would also need handle the Set2dByteArrayVariantByRef...Being able to pass in a multi-dimensional array is important also.

[wyrock]

Logged In: YES
user_id=1681725
Originator: YES

I just wanted to add some follow up here and get some advice....First off, I now see why my original patch was busted. I think someone else tried to warn me but, the data in the SAFEARRAY is not stored in the same order you think of for a traditional C array.

A couple of ideas: instead of leaving it broken, how about just not special casing the multi-dimensional VT_UI1 BYREF array? I'm not asking for a change in the VT_U1 BYREF for a single dimension, just multiple.

OR:

#2 Return a new object (like Mark was suggestion) something with the following attributes:
dimensions = a list, where each entry in the list is (upperbound-lowerbound) for each dimension
bufferdata = the massive buffer of the multi-dimensional array

I like #1 b/c its easy and the buffer is #2 is going to be a bit of pain to convert back in to a multi-dimensional array that makes since to python code. However, #2 may be the preferred method for others. I'm pretty new to the C side of Python, but I'm guessing I could use the ctypes Structure class or the PyRecord class to implement this.

Just thought I'd ask to see if I could get away with #1 or if there was any comments suggestions behind #2.