Re: Bug in tracing in DBD::Oracle

Charles Jardine wrote:
> On 27/01/10 17:38, Martin Evans wrote:
>> Charles Jardine wrote:
>>> On 27/01/10 15:52, Martin Evans wrote:
>>>> Hi,
>>>>
>>>> I was asked to enable ora_verbose and send a trace a few days ago.
>>>>
>>>> I'm getting a segfault with DBD::Oracle when ora_verbose or dbd_verbose
>>>> is set to 15 in the connect method call. The stack trace is:
> 
> [snip]
> 
>>>> and that refers to the following line in dbdimp.c:
>>>>
>>>> OCINlsEnvironmentVariableGet_log_stat( &ncharsetid,(size_t)  0,
>>>> OCI_NLS_NCHARSET_ID, 0, &rsize ,status );
>>>>
>>>> Oracle defines the second argument as size_t so I guess that cast of 0
>>>> to size_t is ok but ocitrace.h then goes on to cast it again to
>>>> (unsigned long long) and the format argument has been changed to %llu.
>>>> Although these match it segfaults.
>>> I am responsible for this change. It was part of a campaign to avoid
>>> warnings
>>> when compiling on 64-bit gcc platforms. All that is necessary to avoid
>>> the compiler warnings is that the format arguments match the casts
>>> (subject to integral promotion).
>>>
>>> I used (unsigned long long) in this case for maximum portability. I
>>> couldn't
>>> find any standard that said that (size_t) might not be wider than
>>> (unsigned long).
>>>
>>> If my change breaks PerlIO_vprintf, we must back off. Using (unsigned
>>> long)
>>> and %lu would work on all platforms I use. Using (unsigned int) and %u,
>>> would work in this case, but not for all uses of size_t.
>>>
>>> This is the only place where I used a %llu or %lld, so there is only
>>> one place to change.
>>>
>>> Martin, can you try changing the casts to (unsigned long) and the
>>> formats
>>> to %lu, and see if this fixes your problem.
>>
>> That is what I did in effect (nearly).
>>
>> I took the casts of 0 to size_t out of the 2 calls in dbdimp.c and added
>> a cast to size_t on the real call to oracle in the macro. Then I change
>> the format in the PerlIO_printf to %lu and change the cast to (unsigned
>> long). This works for me and I guess it will work without warning for
>> you too.
>>
>> This isn't exactly what John has in subversion at the moment.
> 
> John seems to have corrected my over-zealous cast, and produced
> a version which complies without warning and works on both 32-
> and 64-bit platforms. Thank you John.
> 
> I prefer his version, with the cast to site_t left where it was,
> rather than imported into the macro.

I'm not that comfortable with the cast to size_t in dbdimp.c because
then it is later cast back to unsigned long and I'd guess on platforms
where size_t is an unsigned long long the compiler might whine about that.

The best fix would be if there was a reliable format for size_t but I
don't know of one.

However, I don't think we need to get this out of proportion after all
it is only two calls and in both cases the size_t is 0 anyway as the
requested attributes are integers and not strings.

> If the current SVN version works for Martin, I suggest that no
> more needs to be done.

It does work.


> I an sorry to have caused this bother.
> 

Didn't cost me much bother and was done for all the right reasons.

I am considerably more bothered about this serious problem:

oci8.c
======
/*line 1897 */
	if (DBIS->debug >= 3 || dbd_verbose >= 3 || oci_warn){
		char buf[10];
		sprintf(buf,"bytes");

		if (ftype == ORA_CLOB)
           sprintf(buf,"characters");

		PerlIO_printf(DBILOGFP,
		"		OCILobRead %s %s: csform %d (%s), LOBlen %lu(%s), LongReadLen
%lu(%s), BufLen %lu(%s), Got %lu(%s)\n",
				name, oci_status_name(status), csform,oci_csform_name(csform),
ul_t(loblen),buf ,
				ul_t(imp_sth->long_readlen),buf, ul_t(buflen),buf, ul_t(amtp),buf);

    }

That sprintf will always overflow buf as buf is 10 chrs long and does
not have room for the trailing NUL.

The attached patch fixes above and a few other ones I saw.

I suggest we try very hard to get someone with a 64bit platform to try
the next RC.

Martin
-- 
Martin J. Evans
Easysoft Limited
http://www.easysoft.com