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
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!
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.
(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.
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
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
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.