[freetds] failure converting to numeric

[samuel.ferencik]

Hi,

Here's a patch for ctlib/cs.c:

--- src\src\ctlib\cs#1.c 2014-02-05 20:04:52.000000000 +-0100
+++ src\src\ctlib\cs.c 2014-02-05 19:47:01.000000000 +-0100
@@ -566,13 +566,13 @@
destdata = destvc->str;
}
tdsdump_log(TDS_DBG_FUNC, "converting type %d (%d bytes) to type = %d (%d bytes)\n",
src_type, src_len, desttype, destlen);
- if (!is_fixed_type(desttype) && (destlen <= 0)) {
+ if (!is_fixed_type(desttype) && !is_numeric_type(desttype) && (destlen <= 0)) {
return CS_FAIL;
}
dest = (unsigned char *) destdata;
/* many times we are asked to convert a data type to itself */

The change is in cs_convert(). When converting a string to SQL's numeric, the call sequence to_numeric() -> cs_convert() fails if destlen is unset. However, destlen is never needed for the conversion since the conversion is based on the precision & scale of the numeric type.

I came across this when analysing a test failure in Perl's DBD-Sybase test suite.

Any comments? Can I/should I make a merge request on gitorious?

Thanks,
Sam

_______________________________________________

This message is for information purposes only, it is not a recommendation, advice, offer or solicitation to buy or sell a product or service nor an official confirmation of any transaction. It is directed at persons who are professionals and is not intended for retail customer use. Intended for recipient only. This message is subject to the terms at: www.barclays.com/emaildisclaimer.

For important disclosures, please see: www.barclays.com/salesandtradingdisclaimer regarding market commentary from Barclays Sales and/or Trading, who are active market participants; and in respect of Barclays Research, including disclosures relating to specific issuers, please see http://publicresearch.barclays.com.

_______________________________________________

[Frediano Ziglio]

Hi,
good spot!

Documentation state (destfmt->maxlength,
http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.help.mainframeconnect_12.6.occprc/html/occprc/X12978.htm):

dest_maxlen = 35 when converting to numeric or Sybase-decimal.

I think that in case you are converting numeric->numeric and you have
destlen < 0 with your patch code can core trying to do a memcpy(dest,
src, destlen).

Perhaps it's better to just set destlen to 35 before the test for
destlen<=0 is done.

Feel free to send patches in whatever format. Obviously if you want a
specific format, specific name in the patch you should send these
data.

Frediano

Post by samuel.ferencik
Hi,
--- src\src\ctlib\cs#1.c 2014-02-05 20:04:52.000000000 +-0100
+++ src\src\ctlib\cs.c 2014-02-05 19:47:01.000000000 +-0100
@@ -566,13 +566,13 @@
destdata = destvc->str;
}
tdsdump_log(TDS_DBG_FUNC, "converting type %d (%d bytes) to type = %d (%d bytes)\n",
src_type, src_len, desttype, destlen);
- if (!is_fixed_type(desttype) && (destlen <= 0)) {
+ if (!is_fixed_type(desttype) && !is_numeric_type(desttype) && (destlen <= 0)) {
return CS_FAIL;
}
dest = (unsigned char *) destdata;
/* many times we are asked to convert a data type to itself */
The change is in cs_convert(). When converting a string to SQL's numeric, the call sequence to_numeric() -> cs_convert() fails if destlen is unset. However, destlen is never needed for the conversion since the conversion is based on the precision & scale of the numeric type.
I came across this when analysing a test failure in Perl's DBD-Sybase test suite.
Any comments? Can I/should I make a merge request on gitorious?
Thanks,
Sam
_______________________________________________
This message is for information purposes only, it is not a recommendation, advice, offer or solicitation to buy or sell a product or service nor an official confirmation of any transaction. It is directed at persons who are professionals and is not intended for retail customer use. Intended for recipient only. This message is subject to the terms at: www.barclays.com/emaildisclaimer.
For important disclosures, please see: www.barclays.com/salesandtradingdisclaimer regarding market commentary from Barclays Sales and/or Trading, who are active market participants; and in respect of Barclays Research, including disclosures relating to specific issuers, please see http://publicresearch.barclays.com.
_______________________________________________
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds

[samuel.ferencik]

I agree. There are still two options, though:

1) change the patch as so:

--- src\src\ctlib\cs#1.c 2014-02-07 13:51:17.000000000 +-0100
+++ src\src\ctlib\cs.c 2014-02-07 13:49:45.000000000 +-0100
@@ -566,12 +566,16 @@
destdata = destvc->str;
}

tdsdump_log(TDS_DBG_FUNC, "converting type %d (%d bytes) to type = %d (%d bytes)\n",
src_type, src_len, desttype, destlen);

+ if (is_numeric_type(desttype)) {
+ destlen = 35;
+ }
+
if (!is_fixed_type(desttype) && (destlen <= 0)) {
return CS_FAIL;
}

dest = (unsigned char *) destdata;

OR
2) Blame the caller (DBD-Sybase in this case). The documentation suggests that
filling in maxlen is a precondition for cs_convert(). Maybe I should submit
a patch to DBD-Sybase instead (see to_numeric() in dbdimp.c). What do you
think?

Thanks,
Sam

2014-02-05

Post by Frediano Ziglio
Hi,
good spot!
Documentation state (destfmt->maxlength,
dest_maxlen = 35 when converting to numeric or Sybase-decimal.
I think that in case you are converting numeric->numeric and you have
destlen < 0 with your patch code can core trying to do a memcpy(dest,
src, destlen).
Perhaps it's better to just set destlen to 35 before the test for
destlen<=0 is done.
Feel free to send patches in whatever format. Obviously if you want a
specific format, specific name in the patch you should send these
data.
Frediano

_______________________________________________

This message is for information purposes only, it is not a recommendation, advice, offer or solicitation to buy or sell a product or service nor an official confirmation of any transaction. It is directed at persons who are professionals and is not intended for retail customer use. Intended for recipient only. This message is subject to the terms at: www.barclays.com/emaildisclaimer.

For important disclosures, please see: www.barclays.com/salesandtradingdisclaimer regarding market commentary from Barclays Sales and/or Trading, who are active market participants; and in respect of Barclays Research, including disclosures relating to specific issuers, please see http://publicresearch.barclays.com.

_______________________________________________

[Frediano Ziglio]

I would say 1). Does something like
https://gitorious.org/freetds/mars-freetds/commit/5c266c97fbeabca5afd0fdae1c327746c6ce3a89
could work ?

Frediano

Post by samuel.ferencik
--- src\src\ctlib\cs#1.c 2014-02-07 13:51:17.000000000 +-0100
+++ src\src\ctlib\cs.c 2014-02-07 13:49:45.000000000 +-0100
@@ -566,12 +566,16 @@
destdata = destvc->str;
}
tdsdump_log(TDS_DBG_FUNC, "converting type %d (%d bytes) to type = %d (%d bytes)\n",
src_type, src_len, desttype, destlen);
+ if (is_numeric_type(desttype)) {
+ destlen = 35;
+ }
+
if (!is_fixed_type(desttype) && (destlen <= 0)) {
return CS_FAIL;
}
dest = (unsigned char *) destdata;
OR
2) Blame the caller (DBD-Sybase in this case). The documentation suggests that
filling in maxlen is a precondition for cs_convert(). Maybe I should submit
a patch to DBD-Sybase instead (see to_numeric() in dbdimp.c). What do you
think?
Thanks,
Sam
2014-02-05

Post by Frediano Ziglio
Hi,
good spot!
Documentation state (destfmt->maxlength,
dest_maxlen = 35 when converting to numeric or Sybase-decimal.
I think that in case you are converting numeric->numeric and you have
destlen < 0 with your patch code can core trying to do a memcpy(dest,
src, destlen).
Perhaps it's better to just set destlen to 35 before the test for
destlen<=0 is done.
Feel free to send patches in whatever format. Obviously if you want a
specific format, specific name in the patch you should send these
data.
Frediano

_______________________________________________
This message is for information purposes only, it is not a recommendation, advice, offer or solicitation to buy or sell a product or service nor an official confirmation of any transaction. It is directed at persons who are professionals and is not intended for retail customer use. Intended for recipient only. This message is subject to the terms at: www.barclays.com/emaildisclaimer.
For important disclosures, please see: www.barclays.com/salesandtradingdisclaimer regarding market commentary from Barclays Sales and/or Trading, who are active market participants; and in respect of Barclays Research, including disclosures relating to specific issuers, please see http://publicresearch.barclays.com.
_______________________________________________
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds

[samuel.ferencik]

2014-02-08

Post by Frediano Ziglio
I would say 1). Does something like
https://gitorious.org/freetds/mars-freetds/commit/5c266c97fbeabca5afd0fdae1c327746c6ce3a89
could work ?
Frediano

Yes, that's much better. Please go ahead.

Still, I think I'll file a bug with DBD-Sybase in case it uses a different underlying implementation.

Thanks for your help.
Sam

_______________________________________________

This message is for information purposes only, it is not a recommendation, advice, offer or solicitation to buy or sell a product or service nor an official confirmation of any transaction. It is directed at persons who are professionals and is not intended for retail customer use. Intended for recipient only. This message is subject to the terms at: www.barclays.com/emaildisclaimer.

For important disclosures, please see: www.barclays.com/salesandtradingdisclaimer regarding market commentary from Barclays Sales and/or Trading, who are active market participants; and in respect of Barclays Research, including disclosures relating to specific issuers, please see http://publicresearch.barclays.com.

_______________________________________________