You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafodion.apache.org by we...@esgyn.cn on 2018/12/19 10:34:32 UTC
questionable `sprintf` usage
Hi,
I suspect the following code in core/sql/ustat/hs_read.cpp is erroneous:
2120 desc = new SQLDESC_ID;
2121 init_SQLCLI_OBJ_ID(desc);
2122
2123 desc->name_mode = cursor_name;
2124 desc->module = &module;
2125 desc->identifier = new char[HS_STMTID_LENGTH];
2126 desc->handle = 0;
2127
2128 sprintf((char*)desc->identifier, descID);
2129 desc->identifier_len = strlen(descID);
The parameters to function `sprintf` should be 3, but there are only 2.
I’d like to change it to:
snprintf((char*)desc->identifier, HS_STMTID_LENGTH, “%s”, descID);
How do you find it?
Regards,
Wenjun Zhu
Re: questionable `sprintf` usage
Posted by He Zhenxing <zh...@esgyn.cn>.
Hi,
sprintf function can be called with only 2 arguments if there is no
format specifiers in the second argument (template).
So the following two are identical:
char *s = "some string";
sprintf(target, s);
sprintf(target, "%s", s);
But of course, sprintf is usually not recommended because it does not
check for the buffer length, which may lead to buffer overflow and cause
problems that are hard to find.
He Zhenxing
在 2018/12/19 下午6:34, wenjun.zhu@esgyn.cn 写道:
> Hi,
>
>
>
> I suspect the following code in core/sql/ustat/hs_read.cpp is erroneous:
>
> 2120 desc = new SQLDESC_ID;
>
> 2121 init_SQLCLI_OBJ_ID(desc);
>
> 2122
>
> 2123 desc->name_mode = cursor_name;
>
> 2124 desc->module = &module;
>
> 2125 desc->identifier = new char[HS_STMTID_LENGTH];
>
> 2126 desc->handle = 0;
>
> 2127
>
> 2128 sprintf((char*)desc->identifier, descID);
>
> 2129 desc->identifier_len = strlen(descID);
>
>
>
> The parameters to function `sprintf` should be 3, but there are only 2.
>
>
>
> I’d like to change it to:
>
> snprintf((char*)desc->identifier, HS_STMTID_LENGTH, “%s”, descID);
>
>
>
> How do you find it?
>
>
>
> Regards,
>
> Wenjun Zhu
>
>
RE: questionable `sprintf` usage
Posted by Dave Birdsall <da...@esgyn.com>.
Hi,
Just a follow-up. Awhile back Anoop Sharma and myself removed some of the obsolete static SQL code from UPDATE STATISTICS. You can find a reference here: https://issues.apache.org/jira/browse/TRAFODION-3138 (the pull request is here https://github.com/apache/trafodion/pull/1639). There is still some left to be removed, including, apparently, this OpenCursor method. Feel free to open a new JIRA and remove the rest of the static SQL code if you wish.
Dave
-----Original Message-----
From: Dave Birdsall <da...@esgyn.com>
Sent: Wednesday, December 19, 2018 9:22 AM
To: dev@trafodion.apache.org
Subject: RE: questionable `sprintf` usage
External
Hi,
I think this is dead code, actually. I looked for callers to OpenCursor but did not find any.
The comment at the top of the function says that it is used for static modules. Static SQL compilation was supported in Trafodion's predecessor product, but Trafodion itself does not support it.
We probably can remove this function.
Dave
-----Original Message-----
From: Selva Govindarajan <se...@esgyn.com>
Sent: Wednesday, December 19, 2018 7:38 AM
To: dev@trafodion.apache.org
Subject: RE: questionable `sprintf` usage
External
The code is not erroneous, though it is bit strange.
Declaration of sprintf is
int sprintf ( char * str, const char * format, ... );
It just needs 2 parameters, the rest are optional. In this case when format parameter has no format specification, sprintf just copies the format parameter to str.
Trafodion code is compiled with -Wformat -Werror. This should emit out compilation errors when printf, sprintf is used in incorrect way such as less number of arguments than the required number as per the format specification, incompatible format and argument, and other errors.
snprintf might be good to avoid buffer overflow, but in this case I am not sure if there was a buffer overflow condition.
Selva
-----Original Message-----
From: wenjun.zhu@esgyn.cn <we...@esgyn.cn>
Sent: Wednesday, December 19, 2018 2:35 AM
To: dev@trafodion.apache.org
Subject: questionable `sprintf` usage
Hi,
I suspect the following code in core/sql/ustat/hs_read.cpp is erroneous:
2120 desc = new SQLDESC_ID;
2121 init_SQLCLI_OBJ_ID(desc);
2122
2123 desc->name_mode = cursor_name;
2124 desc->module = &module;
2125 desc->identifier = new char[HS_STMTID_LENGTH];
2126 desc->handle = 0;
2127
2128 sprintf((char*)desc->identifier, descID);
2129 desc->identifier_len = strlen(descID);
The parameters to function `sprintf` should be 3, but there are only 2.
I’d like to change it to:
snprintf((char*)desc->identifier, HS_STMTID_LENGTH, “%s”, descID);
How do you find it?
Regards,
Wenjun Zhu
RE: questionable `sprintf` usage
Posted by Dave Birdsall <da...@esgyn.com>.
Hi,
I think this is dead code, actually. I looked for callers to OpenCursor but did not find any.
The comment at the top of the function says that it is used for static modules. Static SQL compilation was supported in Trafodion's predecessor product, but Trafodion itself does not support it.
We probably can remove this function.
Dave
-----Original Message-----
From: Selva Govindarajan <se...@esgyn.com>
Sent: Wednesday, December 19, 2018 7:38 AM
To: dev@trafodion.apache.org
Subject: RE: questionable `sprintf` usage
External
The code is not erroneous, though it is bit strange.
Declaration of sprintf is
int sprintf ( char * str, const char * format, ... );
It just needs 2 parameters, the rest are optional. In this case when format parameter has no format specification, sprintf just copies the format parameter to str.
Trafodion code is compiled with -Wformat -Werror. This should emit out compilation errors when printf, sprintf is used in incorrect way such as less number of arguments than the required number as per the format specification, incompatible format and argument, and other errors.
snprintf might be good to avoid buffer overflow, but in this case I am not sure if there was a buffer overflow condition.
Selva
-----Original Message-----
From: wenjun.zhu@esgyn.cn <we...@esgyn.cn>
Sent: Wednesday, December 19, 2018 2:35 AM
To: dev@trafodion.apache.org
Subject: questionable `sprintf` usage
Hi,
I suspect the following code in core/sql/ustat/hs_read.cpp is erroneous:
2120 desc = new SQLDESC_ID;
2121 init_SQLCLI_OBJ_ID(desc);
2122
2123 desc->name_mode = cursor_name;
2124 desc->module = &module;
2125 desc->identifier = new char[HS_STMTID_LENGTH];
2126 desc->handle = 0;
2127
2128 sprintf((char*)desc->identifier, descID);
2129 desc->identifier_len = strlen(descID);
The parameters to function `sprintf` should be 3, but there are only 2.
I’d like to change it to:
snprintf((char*)desc->identifier, HS_STMTID_LENGTH, “%s”, descID);
How do you find it?
Regards,
Wenjun Zhu
RE: questionable `sprintf` usage
Posted by Selva Govindarajan <se...@esgyn.com>.
The code is not erroneous, though it is bit strange.
Declaration of sprintf is
int sprintf ( char * str, const char * format, ... );
It just needs 2 parameters, the rest are optional. In this case when format parameter has no format specification, sprintf just copies the format parameter to str.
Trafodion code is compiled with -Wformat -Werror. This should emit out compilation errors when printf, sprintf is used in incorrect way such as less number of arguments than the required number as per the format specification, incompatible format and argument, and other errors.
snprintf might be good to avoid buffer overflow, but in this case I am not sure if there was a buffer overflow condition.
Selva
-----Original Message-----
From: wenjun.zhu@esgyn.cn <we...@esgyn.cn>
Sent: Wednesday, December 19, 2018 2:35 AM
To: dev@trafodion.apache.org
Subject: questionable `sprintf` usage
Hi,
I suspect the following code in core/sql/ustat/hs_read.cpp is erroneous:
2120 desc = new SQLDESC_ID;
2121 init_SQLCLI_OBJ_ID(desc);
2122
2123 desc->name_mode = cursor_name;
2124 desc->module = &module;
2125 desc->identifier = new char[HS_STMTID_LENGTH];
2126 desc->handle = 0;
2127
2128 sprintf((char*)desc->identifier, descID);
2129 desc->identifier_len = strlen(descID);
The parameters to function `sprintf` should be 3, but there are only 2.
I’d like to change it to:
snprintf((char*)desc->identifier, HS_STMTID_LENGTH, “%s”, descID);
How do you find it?
Regards,
Wenjun Zhu