FreeTDS Development Group

[LacaK <lacak AT zoznam.sk>]

Frediano Ziglio  wrote / napísal(a):
> 2014-04-09 6:40 GMT+01:00 LacaK <lacak AT zoznam.sk>:
>   
> > Frediano Ziglio  wrote / napísal(a):
> >
> >     
> > > 2014-04-08 11:08 GMT+01:00 LacaK <lacak AT zoznam.sk>:
> > >
> > >       
> > > > Hi,
> > > > when I look into tds_types.h (which is build on misc/types.csv) there is
> > > > function tds_get_size_by_type() used f.e. by dbconvert()
> > > > And for SYBMSDATE it returns 4 (IMO it is wrong set size=4 in types.csv)
> > > > Is it correct ?
> > > > (as far as I understand it should be size if struct which holds this data
> > > > type i.e.:
> > > > there should be:
> > > >  case SYMBSDATE:
> > > >  case SYMBSTIME:
> > > >  case SYMBSDATETIME2:
> > > >  case SYMBSDATETIMEOFFSET:
> > > >     return sizeof(TDS_DATETIMEALL);
> > > >
> > > > ?
> > > > When I change it this way, it works as expected for me.
> > > > Thanks
> > > > -Laco.
> > > >
> > > >
> > > >         
> > >
> > > Well... could be... this function is used in a lot of places. As the
> > > fixed column is 0 I don't think should break anything.
> > >
> > > What did go wrong before you changed the code?
> > >
> > >       
> > Look please in src/dblib/dblib.c at dbconvert() function.
> > There is call to tds_get_size_by_type(desttype) used in case when "srctype
> > == desttype"
> > And then return value (i.e. size) is used in: memcpy(dest, src, ret);
> > When size is wrong memcpy does not copy all data.
> > When tds_get_size_by_type() does not known "desttype" then -1 is returned,
> > which in my environment causes freezing application.
> >     
>
> Well, there is a large switch that prevent the call of
> tds_get_size_by_type for new MS date/time types so you have probably
> change this switch and now the code does not work for you. Is it
> right?
>   

Yes exactly (see attached dblib.c.diff for illustration).
I have added:
 case SYMBSDATE:
 case SYMBSTIME:
 case SYMBSDATETIME2:
 case SYMBSDATETIMEOFFSET:
before:
 ret = tds_get_size_by_type(desttype);


>   
> > (may be that memcpy expect as 3rd argument "unsigned size", so supplying -1
> > causes copy of large memory block)
> > (f.e. for desttype=SYBMSDATETIME2 there is in types.csv: size=-1, which
> > causes that this type is omited when generating tds_types.h from types.pl
> > ... )
> >
> > As another example please look at SYBDATETIME, there is size=4 which
> > correspondes to sizeof(TDS_DATETIME) and also sizeof(DBDATETIME) used by
> > sybdb.h ... this is IMO correct ...
> >
> > Sizeof(TDS_DATETIMEALL) = 24 so setting this size for MSDATE, MSTIME,
> > MSDATETIME2, MSDATETIMEOFFSET should ge solution?
> >
> >     
>
> 24 ?? On my 64 bit machine is 16 and should be 16. However you can use
> directly sizeof(TDS_DATETIMEALL) in csv file.
>   

Hm, may be that MS Visual C++ 2005 uses different padding or aligning (I 
use 32 bit version) ...
(In parael I work also in FreePascal, where I have create "header unit" 
which binds to dblib.dll, so I have translated "structs" to "records" 
etc. and also in FreePascal (in my translated struct -> record) 
sizeof(TDS_DATETIMEALL) = 24)

When I look at TDS_DATETIME there is:
TDS_UINT8   time;
TDS_INT      date;
TDS_SMALLINT offset;
unsigned     time_prec:4;
unsigned     has_time:1;
unsigned     has_date:1;
unsigned     has_offset:1;

I am mainly Pascal programer, so my C++ knowledge is limited, but:
8 (for time) + 4 (for date) + 2 (for offset) + 4 (for unsigned bitfield) 
= at least 18 if no padding occurs.
I do not know from where comes 24, but it is so in my case.

For me is critical, that size of structures between FreePascal and 
compiled (from C/C++ sources) dynamic library will match.
So I do not know how to fix it ... or why there is difference ... can it 
be compiler dependant (that one compiler uses one aligning and other 
compiler uses other aligning)? Can there be used some #pragma to 
explicitly force some alignment ?
(I do not have problem with other structures like DBDATETIME, DBDATEREC, 
DBCOL)


> Well, questions that raise on my mind are due to the use of this
> function and what does it mean the "size" of tds_get_size_by_type.
> I'll explain. Is size the size of the network bytes we receive from
> the server or is the size of the structure stored by libTDS for this
> type? Actually the table reply with the first (network bytes) but you
> are changing with the last (structure bytes). The problem is really
> this, the function is used for both purposes :(
>   

Aha!
I have looked at dbdatlen() (colinfo->column_cur_size) which should 
return size of data returned  by dbdata() (colinfo->column_data)
Then I have looked how is initialised column_cur_size and I have found, 
that in data.c is used in one place also tds_get_size_by_type()
and also in dblib.c is used  tds_get_size_by_type() when manipulating 
with "local structures"
So it leads me to wrong impression, that it is size of "local structure" 
... and as you wrote it was wrong assumption ;-(


> For this reason I think I found a bug in libTDS. If you return a
> VARIANT type from the conversion of these new types I don't know what
> can happen (see src/tds/data.c).
>
> Yesterday I replaced the size in the csv files and do a check. With
> tds 7.3 I had a lot of problems with some internal checks. Well...
> actually removing the test seems to not cause any problem, perhaps
> they are only too strict. However all the issue that can happen not
> considering the above problem could lead to different corruptions :(
>   

So what to do now ?
I can relative easy fix it in dbconvert(), but there will possibly be 
other places which should be fixed.
Do you think, that you can introduce at libTDS level any "new" function 
which will return size of local "TDS_" structures for given data type 
(not on wire size) ?
(then other libraries like dblib, ctlib can reuse this function)

-Laco.

--- dblib.c.ori Thu Apr 03 13:39:42 2014
+++ dblib.c     Wed Apr 09 11:59:04 2014
@@ -2321,6 +2321,10 @@ dbconvert(DBPROCESS * dbproc, int srctyp
                case SYBDATETIME:
                case SYBDATETIME4:
                case SYBUNIQUE:
+               case SYBMSDATE:
+               case SYBMSTIME:
+               case SYBMSDATETIME2:
+               case SYBMSDATETIMEOFFSET:
                        ret = tds_get_size_by_type(desttype);
                        memcpy(dest, src, ret);
                        break;
@@ -2447,6 +2451,13 @@ dbconvert(DBPROCESS * dbproc, int srctyp
                memcpy(dest, &(dres.u), sizeof(TDS_UNIQUE));
                ret = sizeof(TDS_UNIQUE);
                break;
+       case SYBMSDATE:
+       case SYBMSTIME:
+       case SYBMSDATETIME2:
+       case SYBMSDATETIMEOFFSET:
+               memcpy(dest, &(dres.dta), sizeof(TDS_DATETIMEALL));
+               ret = sizeof(TDS_DATETIMEALL);
+               break;
        case SYBCHAR:
        case SYBVARCHAR:
        case SYBTEXT:
@@ -7166,6 +7177,11 @@ tds_prdatatype(TDS_SERVER_TYPE datatype_
        case SYBUINT8:          return "SYBUINT8";
        case SYBUNIQUE:         return "SYBUNIQUE";
        case SYBVARIANT:        return "SYBVARIANT";
+       case SYBMSXML:          return "SYBMSXML";
+       case SYBMSDATE:         return "SYBMSDATE";
+       case SYBMSTIME:         return "SYBMSTIME";
+       case SYBMSDATETIME2: return "SYBMSDATETIME2";
+       case SYBMSDATETIMEOFFSET: return "SYBMSDATETIMEOFFSET";
        default: break;
        }
        return "(unknown)";