Обсуждение: Potential issue in ecpg-informix decimal converting functions
Hi, everyone! I found a potential bug in dectoint() and dectolong() functions from informix.c. "Informix Compatibility Mode" doc chapter says that ECPG_INFORMIX_NUM_OVERFLOW is returned if an overflow occurred. But check this line in dectoint() or dectolong() (it is present in both): if (ret == PGTYPES_NUM_OVERFLOW) - condition is always false because PGTYPESnumeric_to_int() and PGTYPESnumeric_to_long() functions return only 0 or -1. So ECPG_INFORMIX_NUM_OVERFLOW can never be returned. I think dectoint(), dectolong() and PGTYPESnumeric_to_int() functions should be a little bit different like in proposing patch. What do you think? The flaw was catched with the help of Svace static analyzer. https://svace.pages.ispras.ru/svace-website/en/ Thank you!
Вложения
> On 22 Feb 2024, at 17:54, a.imamov@postgrespro.ru wrote: > PGTYPESnumeric_to_int() and PGTYPESnumeric_to_long() > functions return only 0 or -1. So ECPG_INFORMIX_NUM_OVERFLOW can never > be returned. Indeed, this looks like an oversight. > I think dectoint(), dectolong() and PGTYPESnumeric_to_int() functions > should be a little bit different like in proposing patch. > What do you think? - Convert a variable to type decimal to an integer. + Convert a variable of type decimal to an integer. While related, this should be committed and backpatched regardless. + int errnum = 0; Stylistic nit, we typically don't initialize a variable which cannot be accessed before being set. Overall the patch looks sane, please register it for the next commitfest to make it's not missed. -- Daniel Gustafsson
Daniel Gustafsson писал(а) 2024-02-23 13:44: >> On 22 Feb 2024, at 17:54, a.imamov@postgrespro.ru wrote: > >> PGTYPESnumeric_to_int() and PGTYPESnumeric_to_long() >> functions return only 0 or -1. So ECPG_INFORMIX_NUM_OVERFLOW can never >> be returned. > > Indeed, this looks like an oversight. > >> I think dectoint(), dectolong() and PGTYPESnumeric_to_int() functions >> should be a little bit different like in proposing patch. >> What do you think? > > - Convert a variable to type decimal to an integer. > + Convert a variable of type decimal to an integer. > While related, this should be committed and backpatched regardless. > > + int errnum = 0; > Stylistic nit, we typically don't initialize a variable which cannot be > accessed before being set. > > Overall the patch looks sane, please register it for the next > commitfest to > make it's not missed. > > -- > Daniel Gustafsson Thank you for feedback, - Convert a variable to type decimal to an integer. + Convert a variable of type decimal to an integer. I removed this from the patch and proposed to pgsql-docs@lists.postgresql.org + int errnum = 0; fixed Thank's for advice, the patch will be registered for the next commitfest. -- Aidar Imamov
Вложения
On Fri, Feb 23, 2024 at 06:03:41PM +0300, a.imamov@postgrespro.ru wrote: > Thank's for advice, the patch will be registered for the next commitfest. The risk looks really minimal to me, but playing with error codes while the logic of the function is unchanged does not strike me as something to backpatch as it could slightly break applications. On HEAD, I'm OK with that. -- Michael
Вложения
> On 24 Feb 2024, at 02:15, Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Feb 23, 2024 at 06:03:41PM +0300, a.imamov@postgrespro.ru wrote: >> Thank's for advice, the patch will be registered for the next commitfest. > > The risk looks really minimal to me, but playing with error codes > while the logic of the function is unchanged does not strike me as > something to backpatch as it could slightly break applications. On > HEAD, I'm OK with that. Yeah, I think this is for HEAD only, especially given the lack of complaints against backbranches. -- Daniel Gustafsson
On Mon, Feb 26, 2024 at 12:28:51AM +0100, Daniel Gustafsson wrote: > Yeah, I think this is for HEAD only, especially given the lack of complaints > against backbranches. Daniel, are you planning to look at that? I haven't done any detailed lookup, but would be happy to do so it that helps. -- Michael
Вложения
> On 27 Feb 2024, at 06:08, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Feb 26, 2024 at 12:28:51AM +0100, Daniel Gustafsson wrote: >> Yeah, I think this is for HEAD only, especially given the lack of complaints >> against backbranches. > > Daniel, are you planning to look at that? I haven't done any detailed > lookup, but would be happy to do so it that helps. I have it on my TODO for the upcoming CF. -- Daniel Gustafsson
On Tue, Feb 27, 2024 at 09:24:25AM +0100, Daniel Gustafsson wrote: > I have it on my TODO for the upcoming CF. Okay, thanks. -- Michael
Вложения
Michael Paquier писал(а) 2024-02-28 02:14: > On Tue, Feb 27, 2024 at 09:24:25AM +0100, Daniel Gustafsson wrote: >> I have it on my TODO for the upcoming CF. > > Okay, thanks. > -- > Michael Greetings! Sorry, I had been waiting for a few days for my cool-off period to end. The patch now is registered to CF in the 'Refactoring' topic. -- Aidar
> On 27 Feb 2024, at 06:08, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Feb 26, 2024 at 12:28:51AM +0100, Daniel Gustafsson wrote: >> Yeah, I think this is for HEAD only, especially given the lack of complaints >> against backbranches. > > Daniel, are you planning to look at that? I haven't done any detailed > lookup, but would be happy to do so it that helps. I had a look at this today and opted for trimming back the patch a bit. Reading the informix docs the functions we are mimicking for compatibility here does not have an underflow returnvalue, so adding one doesn't seem right (or helpful). The attached fixes the return of overflow and leaves it at that, which makes it possible to backpatch since it's fixing the code to match the documented behavior. -- Daniel Gustafsson
Вложения
Daniel Gustafsson писал(а) 2024-03-06 18:03: >> On 27 Feb 2024, at 06:08, Michael Paquier <michael@paquier.xyz> wrote: >> >> On Mon, Feb 26, 2024 at 12:28:51AM +0100, Daniel Gustafsson wrote: >>> Yeah, I think this is for HEAD only, especially given the lack of >>> complaints >>> against backbranches. >> >> Daniel, are you planning to look at that? I haven't done any detailed >> lookup, but would be happy to do so it that helps. > > I had a look at this today and opted for trimming back the patch a bit. > Reading the informix docs the functions we are mimicking for > compatibility here > does not have an underflow returnvalue, so adding one doesn't seem > right (or > helpful). The attached fixes the return of overflow and leaves it at > that, > which makes it possible to backpatch since it's fixing the code to > match the > documented behavior. > > -- > Daniel Gustafsson I agree with the proposed changes in favor of backward compatibility. Also, is it a big deal that the PGTYPESnumeric_to_long() function doesn't exactly match the documentation, compared to PGTYPESnumeric_to_int()? It handles underflow case separately and sets errno to PGTYPES_NUM_UNDERFLOW additionally.
> On 6 Mar 2024, at 20:12, a.imamov@postgrespro.ru wrote: > I agree with the proposed changes in favor of backward compatibility. I went ahead to pushed this after another look. I'm a bit hesitant to backpatch this since there are no reports against it, and I don't have good sense for how ECPG code is tested and maintained across minor version upgrades. If we want to I will of course do so, so please chime in in case there are different and more informed opinions. > Also, is it a big deal that the PGTYPESnumeric_to_long() function doesn't > exactly match the documentation, compared to PGTYPESnumeric_to_int()? It > handles underflow case separately and sets errno to PGTYPES_NUM_UNDERFLOW > additionally. Fixed as well. -- Daniel Gustafsson