Returning non-terminated string in ECPG Informix-compatible function
Greetings, everyone!
While analyzing output of Svace static analyzer [1]https://svace.pages.ispras.ru/svace-website/en/ I've found a bug.
In function intoasc(interval * i, char *str) from file
src/interfaces/ecpg/compatlib/informix.c
we return a non-terminated string since we use memcpy on tmp which is
itself NULL-teminated but
last zero byte is not copied.
The proposed solution is to use strcpy instead, since it is used in all
other functions in informix.c.
The patch is attached.
[1]: https://svace.pages.ispras.ru/svace-website/en/
Oleg Tselebrovskiy, Postgres Pro
Attachments:
informix_null_terminator.patchtext/x-diff; name=informix_null_terminator.patchDownload
diff --git a/src/interfaces/ecpg/compatlib/informix.c b/src/interfaces/ecpg/compatlib/informix.c
index dccf39582da..80d40aa3e09 100644
--- a/src/interfaces/ecpg/compatlib/informix.c
+++ b/src/interfaces/ecpg/compatlib/informix.c
@@ -654,7 +654,7 @@ intoasc(interval * i, char *str)
if (!tmp)
return -errno;
- memcpy(str, tmp, strlen(tmp));
+ strcpy(str, tmp);
free(tmp);
return 0;
}
On Mon, Jan 29, 2024 at 2:17 PM <o.tselebrovskiy@postgrespro.ru> wrote:
Greetings, everyone!
While analyzing output of Svace static analyzer [1] I've found a bug.
In function intoasc(interval * i, char *str) from file
src/interfaces/ecpg/compatlib/informix.c
we return a non-terminated string since we use memcpy on tmp which is
itself NULL-teminated but
last zero byte is not copied.The proposed solution is to use strcpy instead, since it is used in all
other functions in informix.c.The patch is attached.
Can you please add a test case showcasing the bug? I see dttoasc()
uses strcpy(). So there's already a precedence.
--
Best Wishes,
Ashutosh Bapat
Here's the code for bug reproduction:
#include <stdio.h>
#include <stdlib.h>
EXEC SQL INCLUDE pgtypes_interval.h;
EXEC SQL INCLUDE ecpg_informix.h;
EXEC SQL BEGIN DECLARE SECTION;
char dirty_str[100] = "aaaaaaaaa_bbbbbbbb_ccccccccc_ddddddddd_";
interval *interval_ptr;
EXEC SQL END DECLARE SECTION;
int main()
{
interval_ptr = (interval *) malloc(sizeof(interval));
interval_ptr->time = 100000000;
interval_ptr->month = 240;
printf("dirty_str contents before intoasc: %s\n", dirty_str);
intoasc(interval_ptr, dirty_str);
printf("dirty_str contents after intoasc: %s\n", dirty_str);
return 0;
}
And here's the output:
dirty_str contents before intoasc:
aaaaaaaaa_bbbbbbbb_ccccccccc_ddddddddd_
dirty_str contents after intoasc: @ 20 years 1 min 40
secscccc_ddddddddd_
I compiled it with following commands (provided for quicker
reproduction):
/path/to/pgsql/bin/ecpg informix_bug_example.pgc
gcc -I/path/to/pgsql/include -c informix_bug_example.c
gcc -o informix_bug_example informix_bug_example.o -L/path/to/pgsql/lib
-lecpg -lecpg_compat
I've also found at least one project that uses intoasc() in it -
https://github.com/credativ/informix_fdw/
Oleg Tselebrovskiy, Postgres Pro
Greetings again.
I was looking through more static analyzer output and found another
problem.
In ecpg/pgtypeslib/dt_common.c there are 4 calls of pgtypes_alloc.
This function uses calloc and returns NULL if OOM, but we don't check
its
return value and immediately pass it to strcpy, which could lead to
segfault.
I suggest adding a check for a return value since all other calls of
pgtypes_alloc are checked for NULL.
A proposed patch (with previous and current changes) is attached
Oleg Tselebrovskiy, Postgres Pro
Attachments:
ecpg_bugfixes.patchtext/x-diff; name=ecpg_bugfixes.patchDownload
diff --git a/src/interfaces/ecpg/compatlib/informix.c b/src/interfaces/ecpg/compatlib/informix.c
index dccf39582da..80d40aa3e09 100644
--- a/src/interfaces/ecpg/compatlib/informix.c
+++ b/src/interfaces/ecpg/compatlib/informix.c
@@ -654,7 +654,7 @@ intoasc(interval * i, char *str)
if (!tmp)
return -errno;
- memcpy(str, tmp, strlen(tmp));
+ strcpy(str, tmp);
free(tmp);
return 0;
}
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index 99bdc94d6d7..d4ca0cbff6e 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -2659,6 +2659,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
*/
pfmt++;
tmp = pgtypes_alloc(strlen("%m/%d/%y") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%m/%d/%y");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
@@ -2784,6 +2786,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
case 'r':
pfmt++;
tmp = pgtypes_alloc(strlen("%I:%M:%S %p") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%I:%M:%S %p");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
@@ -2792,6 +2796,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
case 'R':
pfmt++;
tmp = pgtypes_alloc(strlen("%H:%M") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%H:%M");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
@@ -2837,6 +2843,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
case 'T':
pfmt++;
tmp = pgtypes_alloc(strlen("%H:%M:%S") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%H:%M:%S");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
On Thu, Feb 15, 2024 at 12:15:40PM +0700, Oleg Tselebrovskiy wrote:
Greetings again.
I was looking through more static analyzer output and found another problem.
In ecpg/pgtypeslib/dt_common.c there are 4 calls of pgtypes_alloc.
This function uses calloc and returns NULL if OOM, but we don't check its
return value and immediately pass it to strcpy, which could lead to
segfault.I suggest adding a check for a return value since all other calls of
pgtypes_alloc are checked for NULL.
Right.
@@ -654,7 +654,7 @@ intoasc(interval * i, char *str)
if (!tmp)
return -errno;- memcpy(str, tmp, strlen(tmp)); + strcpy(str, tmp);
For this stuff, Ashutosh's point was to integrate a test case in the
patch. With the pgc you have posted, most of the work is done, but
this should be added to src/interfaces/ecpg/test/sql/ to add some
automated coverage. See the area for references showing how this is
achieved.
@@ -2837,6 +2843,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d, case 'T': pfmt++; tmp = pgtypes_alloc(strlen("%H:%M:%S") + strlen(pstr) + 1); + if(!tmp) + return 1;
These are obviously wrong as pgtypes_alloc() could return NULL.
--
Michael
Thanks for review!
I added a regression test that is based on code from previous email
New patch is attached
Oleg Tselebrovskiy, Postgres Pro
Attachments:
ecpg_bugfixes_with_test.patchtext/x-diff; name=ecpg_bugfixes_with_test.patchDownload
diff --git a/src/interfaces/ecpg/compatlib/informix.c b/src/interfaces/ecpg/compatlib/informix.c
index dccf39582da..80d40aa3e09 100644
--- a/src/interfaces/ecpg/compatlib/informix.c
+++ b/src/interfaces/ecpg/compatlib/informix.c
@@ -654,7 +654,7 @@ intoasc(interval * i, char *str)
if (!tmp)
return -errno;
- memcpy(str, tmp, strlen(tmp));
+ strcpy(str, tmp);
free(tmp);
return 0;
}
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index 99bdc94d6d7..d4ca0cbff6e 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -2659,6 +2659,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
*/
pfmt++;
tmp = pgtypes_alloc(strlen("%m/%d/%y") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%m/%d/%y");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
@@ -2784,6 +2786,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
case 'r':
pfmt++;
tmp = pgtypes_alloc(strlen("%I:%M:%S %p") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%I:%M:%S %p");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
@@ -2792,6 +2796,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
case 'R':
pfmt++;
tmp = pgtypes_alloc(strlen("%H:%M") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%H:%M");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
@@ -2837,6 +2843,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
case 'T':
pfmt++;
tmp = pgtypes_alloc(strlen("%H:%M:%S") + strlen(pstr) + 1);
+ if(!tmp)
+ return 1;
strcpy(tmp, "%H:%M:%S");
strcat(tmp, pfmt);
err = PGTYPEStimestamp_defmt_scan(&pstr, tmp, d, year, month, day, hour, minute, second, tz);
diff --git a/src/interfaces/ecpg/test/compat_informix/.gitignore b/src/interfaces/ecpg/test/compat_informix/.gitignore
index f97706ba4be..6967ae77cd2 100644
--- a/src/interfaces/ecpg/test/compat_informix/.gitignore
+++ b/src/interfaces/ecpg/test/compat_informix/.gitignore
@@ -4,6 +4,8 @@
/dec_test.c
/describe
/describe.c
+/intoasc
+/intoasc.c
/rfmtdate
/rfmtdate.c
/rfmtlong
diff --git a/src/interfaces/ecpg/test/compat_informix/Makefile b/src/interfaces/ecpg/test/compat_informix/Makefile
index d50fdc29fd1..638b4e0af78 100644
--- a/src/interfaces/ecpg/test/compat_informix/Makefile
+++ b/src/interfaces/ecpg/test/compat_informix/Makefile
@@ -16,7 +16,8 @@ TESTS = test_informix test_informix.c \
rnull rnull.c \
sqlda sqlda.c \
describe describe.c \
- charfuncs charfuncs.c
+ charfuncs charfuncs.c \
+ intoasc intoasc.c
all: $(TESTS)
diff --git a/src/interfaces/ecpg/test/compat_informix/intoasc.pgc b/src/interfaces/ecpg/test/compat_informix/intoasc.pgc
new file mode 100644
index 00000000000..d13c83bb7a7
--- /dev/null
+++ b/src/interfaces/ecpg/test/compat_informix/intoasc.pgc
@@ -0,0 +1,21 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "pgtypes_interval.h"
+
+EXEC SQL BEGIN DECLARE SECTION;
+ char dirty_str[100] = "aaaaaaaaa_bbbbbbbb_ccccccccc_ddddddddd_";
+ interval *interval_ptr;
+EXEC SQL END DECLARE SECTION;
+
+int main()
+{
+ interval_ptr = (interval *) malloc(sizeof(interval));
+ interval_ptr->time = 100000000;
+ interval_ptr->month = 240;
+
+ printf("dirty_str contents before intoasc: %s\n", dirty_str);
+ intoasc(interval_ptr, dirty_str);
+ printf("dirty_str contents after intoasc: %s\n", dirty_str);
+ return 0;
+}
diff --git a/src/interfaces/ecpg/test/compat_informix/meson.build b/src/interfaces/ecpg/test/compat_informix/meson.build
index e2f8802330d..7e4790933ad 100644
--- a/src/interfaces/ecpg/test/compat_informix/meson.build
+++ b/src/interfaces/ecpg/test/compat_informix/meson.build
@@ -4,6 +4,7 @@ pgc_files = [
'charfuncs',
'dec_test',
'describe',
+ 'intoasc',
'rfmtdate',
'rfmtlong',
'rnull',
diff --git a/src/interfaces/ecpg/test/ecpg_schedule b/src/interfaces/ecpg/test/ecpg_schedule
index 39814a39c17..f9c0a0e3c00 100644
--- a/src/interfaces/ecpg/test/ecpg_schedule
+++ b/src/interfaces/ecpg/test/ecpg_schedule
@@ -7,6 +7,7 @@ test: compat_informix/sqlda
test: compat_informix/describe
test: compat_informix/test_informix
test: compat_informix/test_informix2
+test: compat_informix/intoasc
test: compat_oracle/char_array
test: connect/test2
test: connect/test3
diff --git a/src/interfaces/ecpg/test/expected/compat_informix-intoasc.c b/src/interfaces/ecpg/test/expected/compat_informix-intoasc.c
new file mode 100644
index 00000000000..30988809e92
--- /dev/null
+++ b/src/interfaces/ecpg/test/expected/compat_informix-intoasc.c
@@ -0,0 +1,40 @@
+/* Processed by ecpg (regression mode) */
+/* These include files are added by the preprocessor */
+#include <ecpglib.h>
+#include <ecpgerrno.h>
+#include <sqlca.h>
+/* Needed for informix compatibility */
+#include <ecpg_informix.h>
+/* End of automatic include section */
+#define ECPGdebug(X,Y) ECPGdebug((X)+100,(Y))
+
+#line 1 "intoasc.pgc"
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "pgtypes_interval.h"
+
+/* exec sql begin declare section */
+
+
+
+#line 7 "intoasc.pgc"
+ char dirty_str [ 100 ] = "aaaaaaaaa_bbbbbbbb_ccccccccc_ddddddddd_" ;
+
+#line 8 "intoasc.pgc"
+ interval * interval_ptr ;
+/* exec sql end declare section */
+#line 9 "intoasc.pgc"
+
+
+int main()
+{
+ interval_ptr = (interval *) malloc(sizeof(interval));
+ interval_ptr->time = 100000000;
+ interval_ptr->month = 240;
+
+ printf("dirty_str contents before intoasc: %s\n", dirty_str);
+ intoasc(interval_ptr, dirty_str);
+ printf("dirty_str contents after intoasc: %s\n", dirty_str);
+ return 0;
+}
diff --git a/src/interfaces/ecpg/test/expected/compat_informix-intoasc.stderr b/src/interfaces/ecpg/test/expected/compat_informix-intoasc.stderr
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/src/interfaces/ecpg/test/expected/compat_informix-intoasc.stdout b/src/interfaces/ecpg/test/expected/compat_informix-intoasc.stdout
new file mode 100644
index 00000000000..0769465b840
--- /dev/null
+++ b/src/interfaces/ecpg/test/expected/compat_informix-intoasc.stdout
@@ -0,0 +1,2 @@
+dirty_str contents before intoasc: aaaaaaaaa_bbbbbbbb_ccccccccc_ddddddddd_
+dirty_str contents after intoasc: @ 20 years 1 min 40 secs
On Thu, Feb 15, 2024 at 05:17:17PM +0700, Oleg Tselebrovskiy wrote:
Thanks for review!
dt_common.c is quite amazing, the APIs that we have in it rely on
strcpy() but we have no idea of the length of the buffer string given
in input to store the result. This would require breaking the
existing APIs or inventing new ones to be able to plug some safer
strlcpy() calls. Not sure if it's really worth bothering. For now,
I've applied the OOM checks on HEAD and the fix with the null
termination on all stable branches.
--
Michael