On Sat Jul 11 08:36:39 2015, ZHENYZ wrote:
Show quoted text> Perl 5.20.1, ASE 15.7, gcc 4.8
>
> main.t gets over "ok 31 - execute compute" and crashes on fetching
> compute results.
> The test log is like
>
> ok 30 - Prepare compute
> ok 31 - execute compute
> Failed 5/36 subtests
>
> I looked at this issue with valgrind, it shows that the segfault
> happens at line 4092 of dbdimp.c.
>
> ==5230==
> syb_st_fetch() -> 2/0/0
> ==5230== Invalid read of size 4
> ==5230== at 0x40A86A0: syb_st_fetch (dbdimp.c:4092)
> ==5230== by 0x4098CD8: XS_DBD__Sybase__st_fetchrow_arrayref
> (Sybase.xsi:630)
>
> Corresponding source of dbdimp.c:
>
> 4073 for (i = 0; i < num_fields; ++i) {
> 4074 SV *sv = AvARRAY(av)[i]; /* Note: we (re)use the SV
> in the AV */
> 4075 len = 0;
> 4076
> 4077 if (DBIc_DBISTATE(imp_dbh)->debug >= 5) {
> 4078 /*char *text = neatsvpv(phs->sv,0);*/
> 4079 PerlIO_printf(DBIc_LOGPIO(imp_dbh),
> 4080 " syb_st_fetch() -> %d/%d/%d\n", i,
> 4081 imp_sth->coldata[i].valuelen, imp_sth-
> >coldata[i].type);
> 4082 }
> 4083 /* If we're beyond the number of items in this result
> set
> 4084 or: the data is null
> 4085 or: noBindBlob is set and the data type is IMAGE or
> TEXT
> 4086 then: set sv to undef */
> 4087 if (i >= imp_sth->numCols || imp_sth-
> >coldata[i].indicator
> 4088 == CS_NULLDATA || (imp_sth->noBindBlob
> 4089 && (imp_sth->datafmt[i].datatype ==
> CS_TEXT_TYPE
> 4090 || imp_sth->datafmt[i].datatype ==
> CS_IMAGE_TYPE))) {
> 4091 /* NULL data */
> 4092 (void) SvOK_off(sv);
> 4093 } else {
>
> There is a loop to go through the columns. And the problem is that the
> loop index i goes out of bound of array av, which comes from below
> assignment.
>
> 4059 av = DBIc_DBISTATE(imp_dbh)->get_fbav(imp_sth);
>
> Before the rows for compute results, a data row has 4 columns. When
> the data rows end, it saw compute rows of 2 columns and reallocs to
> shrink the av array for 2 columns. Below log was dumped with env
> variable DBI_TRACE=5 to indicate the realloc.
>
> syb_st_fetch() -> ct_fetch() = -204 (0 rows, 4 cols)
> st_next_result() -> ct_results(4045) == 1
> ct_res_info() returns 2 columns
> STORE DBI::st=HASH(0xa784410) 'NUM_OF_FIELDS' => 2
> dbih_setup_fbav realloc from 4 to 2 fields
>
> But the variable num_fields at dbdimp.c line 4073 is not changed and
> remains 4. This causes loop index i to go out of the array bound.
>
> I also tried another Perl we have, that is 5.14.2 (compiler is gcc
> 3.4), it also goes out of the bound here, but it does not have
> segfault, and main.t can pass. Below DBIC_TRACE=5 log shows that it
> can run to i=3 for the loop index.
>
> syb_st_fetch() -> 2/0/0
> syb_st_fetch() -> 3/0/0
> ...
>
> For 5.20, we cannot see that line of "syb_st_fetch() -> 3/0/0" in log,
> as segfault happens right after that 2/0/0 line.
The same thing happens in 5.22. I've performed some more analysis.
The following code snippets are from DBD::Sybase 1.16.
When the regular rows are done, the next call to syb_st_fetch() will result in
4059
4060 TryAgain: retcode = ct_fetch(cmd, CS_UNUSED, CS_UNUSED, CS_UNUSED,
4061 &rows_read);
4062
setting retcode = CS_END_DATA. This is then processed here:
4206 case CS_END_DATA: /* we've seen all the data for this result
4207 set. So see if this is the end of the
4208 result sets */
4209
4210 restype = st_next_result(sth, imp_sth);
4211 if (DBIc_DBISTATE(imp_dbh)->debug >= 3)
4212 PerlIO_printf(DBIc_LOGPIO(imp_dbh),
4213 " syb_st_fetch() -> st_next_results() == %d\n", restype);
4214
4215 if (restype == CS_CMD_DONE || restype == CS_CMD_FAIL) {
4216 return Nullav;
4217 } else { /* XXX What to do here??? */
4218 /* if(fix_fbav(imp_sth, num_fields, av))
4219 clear_cache(sth, imp_sth);*/
4220
4221 if (restype == CS_COMPUTE_RESULT) {
4222 goto TryAgain;
4223 }
4224
4225 imp_sth->moreResults = 1;
4226 }
4227 return Nullav;
4228 break;
The call to st_next_result at line 4210 is what leads to the reallocation of av. In st_next_result,
3343 while ((retcode = ct_results(cmd, &restype)) == CS_SUCCEED) {
3344 if (DBIc_DBISTATE(imp_dbh)->debug >= 3)
3345 PerlIO_printf(DBIc_LOGPIO(imp_dbh),
3346 " st_next_result() -> ct_results(%d) == %d\n", restype,
3347 retcode);
3348
3349 if (restype == CS_CMD_FAIL)
3350 failFlag = 1;
3351 if ((restype == CS_CMD_DONE || restype == CS_CMD_SUCCEED) && !failFlag) {
3352 ct_res_info(cmd, CS_ROW_COUNT, &imp_sth->numRows, CS_UNUSED, NULL);
3353 }
3354 switch (restype) {
3355 case CS_ROW_RESULT:
3356 case CS_PARAM_RESULT:
3357 case CS_STATUS_RESULT:
3358 case CS_CURSOR_RESULT:
3359 case CS_COMPUTE_RESULT:
3360 if (imp_sth->done_desc) {
3361 cleanUp(imp_sth);
3362 clear_cache(sth, imp_sth);
3363 }
3364 retcode = describe(sth, imp_sth, restype);
the call to ct_results at line 3343 sets restype to CS_COMPUTE_RESULT.
The switch( restype) at 3354 leads to the invocation of describe() at line 3364. And in describe(),
3064 /* According to Tim Bunce I shouldn't need the code below.
3065 However, if I remove it DBD::Sybase segfaults in some situations
3066 with DBI < 1.53, and there are still problems with COMPUTE BY
3067 statements with DBI >= 1.54. */
3068 /* Adjust NUM_OF_FIELDS - which also adjusts the row buffer size */
3069 DBIc_NUM_FIELDS(imp_sth) = 0; /* for DBI <= 1.53 */
3070 DBIc_DBISTATE(imp_sth)->set_attr_k(sth, sv_2mortal(
3071 newSVpvn("NUM_OF_FIELDS", 13)), 0, sv_2mortal(newSViv(numCols)));
3072
And now we need to switch distributions, to DBI (version 1.637). In DBI.xs, in the set_attr_k function,
2246 else if (htype==DBIt_ST && strEQ(key, "NUM_OF_FIELDS")) {
2247 D_imp_sth(h);
2248 int new_num_fields = (SvOK(valuesv)) ? SvIV(valuesv) : -1;
2249 DBIc_NUM_FIELDS(imp_sth) = new_num_fields;
2250 if (DBIc_FIELDS_AV(imp_sth)) { /* modify existing fbav */
2251 dbih_setup_fbav(imp_sth);
2252 }
the changed number of fields is noted, and dbih_setup_fbav() (same file) performs the reallocation:
1803 if (av) {
1804 if (av_len(av)+1 == i) /* is existing array the right size? */
1805 return av;
1806 /* we need to adjust the size of the array */
1807 if (DBIc_TRACE_LEVEL(imp_sth) >= 2)
1808 PerlIO_printf(DBIc_LOGPIO(imp_sth)," dbih_setup_fbav realloc from %ld to %ld fields\n", (long)(av_len(av)+1), (long)i);
1809 SvREADONLY_off(av);
1810 if (i < av_len(av)+1) /* trim to size if too big */
1811 av_fill(av, i-1);
1812 }
Eventually (!) the control flow returns to syb_st_fetch, line 4211.
4210 restype = st_next_result(sth, imp_sth);
4211 if (DBIc_DBISTATE(imp_dbh)->debug >= 3)
4212 PerlIO_printf(DBIc_LOGPIO(imp_dbh),
4213 " syb_st_fetch() -> st_next_results() == %d\n", restype);
4214
4215 if (restype == CS_CMD_DONE || restype == CS_CMD_FAIL) {
4216 return Nullav;
We pick up the story again at line 4217. Note that the author expresses concern about what to do, and
the fix_fbav() function, which is commented out was an attempt to perform some sort of a fix.
4217 } else { /* XXX What to do here??? */
4218 /* if(fix_fbav(imp_sth, num_fields, av))
4219 clear_cache(sth, imp_sth);*/
4220
4221 if (restype == CS_COMPUTE_RESULT) {
4222 goto TryAgain;
4223 }
4224
4225 imp_sth->moreResults = 1;
4226 }
4227 return Nullav;
4228 break;
Regardless, at line 4222 we jump back to TryAgain (line 4060), with num_fields now no longer tracking
the revised size of av.
Here's my incredibly simple-minded fix (attached), based upon the following logic. In the describe()
function, which the new number of columns is determined from ct_res_info:
3049 if ((retcode = ct_res_info(imp_sth->cmd, CS_NUMDATA, &numCols, CS_UNUSED,
3050 NULL)) != CS_SUCCEED) {
in the same way that num_fields is determined in syb_st_fetch:
4050 ** Find out how many columns there are in this result set.
4051 */
4052 retcode = ct_res_info(cmd, CS_NUMDATA, &num_fields, CS_UNUSED, NULL);
describe() later stores this in imp_sth->numCols:
3085
3086 imp_sth->numCols = numCols;
3087
So, since the number of columns is determined in the same way, rather than calling ct_res_info again,
I simply set num_fields to imp_sth->numCols prior to the jump to TryAgain:
4221 if (restype == CS_COMPUTE_RESULT) {
4222 num_fields = imp_sth->numCols;
4223 goto TryAgain;
4224 }
4225
and the tests pass.
