Bug 472692 - [PATCH] Fixes in odbc.c to return size of returned strings and handle OLE data
[PATCH] Fixes in odbc.c to return size of returned strings and handle OLE data
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: mdbtools (Show other bugs)
10
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-23 10:40 EST by Even Rouault
Modified: 2008-11-26 01:21 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-26 01:21:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Fixes in odbc.c to return size of returned strings and handle OLE data (1.43 KB, patch)
2008-11-23 10:40 EST, Even Rouault
no flags Details | Diff
Updated patch with odbc.c fixes (2.42 KB, patch)
2008-11-24 04:21 EST, Hans de Goede
no flags Details | Diff

  None (edit)
Description Even Rouault 2008-11-23 10:40:19 EST
Created attachment 324435 [details]
Fixes in odbc.c to return size of returned strings and handle OLE data

Description of problem:

Attached a patch that fixes several issues in odbc.c that have been found
while using libmdbodbc.so with the OGR PGeo driver of the GDAL package.
- SQLDescribeCol doesn't return the string length in *pcbColName, which
  breaks CPLODBCStatement::CollectResultsInfo() in port/cpl_odbc.cpp in
  GDAL that uses that value to force a '\0' character at the end of the
  string
- SQLColAttributes doesn't return the string length in *pcbDesc, which
  breaks CPLODBCStatement::CollectResultsInfo() in port/cpl_odbc.cpp in
  GDAL that uses that value to force a '\0' character at the end of the
  string
- SQLGetData doesn't return the correct string length in *pcbValue. It
  should return strlen(rgbValue) instead of col->cur_value_len which is
  sometimes < strlen(rgbValue). That breaks CPLODBCStatement::Fetch in
  port/cpl_odbc.cpp that uses that value to force a '\0' character at
  the end of the string
- SQLGetData doesn't handle correctly OLE data type, which prevents the
  OGR PGeo driver from retrieving the geometry data


Version-Release number of selected component (if applicable):
0.5.99.0.6pre1.0.20051109

How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:

Due to the lack of upstream life, this bug has also been reported to Debian and Ubuntu
Comment 1 Hans de Goede 2008-11-24 04:21:54 EST
Created attachment 324459 [details]
Updated patch with odbc.c fixes

Thanks for the patch, here is an updated version with some changes, can you please review and test this? I'm not all that familiar with odbc.

Changes:
1) Fix your use of strncpy.
   strncpy is a dangerous function to use as if the source is longer then dest
   it will not 0 terminate the copy!
2) While doing 1, I noticed that the original code has several off by one
   errors where it will write one more byte (the 0 terminator) then the 
   application has indicated are available in the buffer. I've checked the API 
   docs here:
   http://msdn.microsoft.com/en-us/library/ms716289.aspx
   And according to that if the string is too long, it should be truncated to
   the given length, *minus* the room necessary for the 0 terminator
3) While doing 2 and reading the docs, I noticed that the returned length should
   be what would have been written if the buffer is long enough, not what is
   actually written, so my new patch fixes this in your patch too.

Once I've got an ok from you on the updated patch, I plan on building a version with the patch in for devel and F-10, will you be needing this in F-9 too?

Also can you please give this updated patch to the other distro's where you submitted it? Thanks!
Comment 2 Even Rouault 2008-11-24 17:46:49 EST
Hans,

thanks for your review. I'm not an odbc expert at all either. I think this patch should not be considered more than a band-aid fix to make GDAL work with MDB/ODBC... to some extent.

I've tested successfully your revised version of the patch.

I see how you have adressed points 1) and 2) in your version of the patch, but not for point 3). Anyway, my understanding of the MS doc is that point 3) is relevant for SQLGetData() only and that would imply that the mdbtools implementation should support being called multiple times to set the data by chunks and return SQL_SUCCESS_WITH_INFO in that case, which is not the case.

If you are interested in how that is handled by calling code, you could see CPLODBCStatement::Fetch() in the GDAL source code (in port/cpl_odbc.cpp). This part of GDAL was only tested on Windows I guess...

Adding support for the full semantics of SQLGetData() would require a non trivial amount of work... I also see that the fCType argument is ignored. So if the calling code would query other types than strings as the type for returned data, that would really lead to bad things...

As for in which version it must be applied, I let it up to you. To be honest, I don't use Fedora personnally. In the absence of upstream life, I just wanted that some major distro's be aware of the issue & patch.
Comment 3 Hans de Goede 2008-11-25 03:33:14 EST
(In reply to comment #2)
> I've tested successfully your revised version of the patch.
> 

Thanks, i'll commit and build it now then.

> I see how you have adressed points 1) and 2) in your version of the patch, but
> not for point 3).

I'm returning strlen(foo) instead of namelen. namelen was the MIN of strlen or the buffersize, so if the string was too long, your patch would return the buffersize instead of the length which would have been written if the buffer was large enough. Note that in the rgbValue case you did to right thing as you used strlen there in your initial patch (which was inconsistent with the other 2 cases where you were returning namelen).

> Anyway, my understanding of the MS doc is that point 3) is
> relevant for SQLGetData() only and that would imply that the mdbtools
> implementation should support being called multiple times to set the data by
> chunks and return SQL_SUCCESS_WITH_INFO in that case, which is not the case.
>

Erm, not necessarily, usually things like this are handled by the application creating a bigger (big enough) buffer and then calling the function again, atleast that is how I would do it.
 
> Adding support for the full semantics of SQLGetData() would require a non
> trivial amount of work... I also see that the fCType argument is ignored. So if
> the calling code would query other types than strings as the type for returned
> data, that would really lead to bad things...
> 

Ok, still while fixing the few things your patch touches I thought it would be good to fix them properly.

> As for in which version it must be applied, I let it up to you. To be honest, I
> don't use Fedora personnally. In the absence of upstream life, I just wanted
> that some major distro's be aware of the issue & patch.

Ok, and many many thanks for taking the trouble to report this to various distros! Projects were upstream is dead are a pain.
Comment 4 Fedora Update System 2008-11-25 04:47:40 EST
mdbtools-0.6-0.5.cvs20051109.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/mdbtools-0.6-0.5.cvs20051109.fc10
Comment 5 Bug Zapper 2008-11-26 00:49:21 EST
This bug appears to have been reported against 'rawhide' during the Fedora 10 development cycle.
Changing version to '10'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping
Comment 6 Fedora Update System 2008-11-26 01:21:27 EST
mdbtools-0.6-0.5.cvs20051109.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.