missing check for truncation of numeric data lead to "double free" or other memory errors

[GitMensch]

This is related to the "in progress" issue #31 and should ideally be solved together with that.

Given the code in

Open-COBOL-ESQL/dblib/ocesql.c

Lines 2789 to 3172 in 1b272b8

	case OCDB_TYPE_UNSIGNED_NUMBER:
	{
	char *ptr;
	int fillzero;
	int zcount;
	char *final;
	int finalbuflen;
	// fill zero
	finalbuflen = sv->length + TERMINAL_LENGTH;
	final = (char *)calloc(finalbuflen, sizeof(char));
	// before decimal point
	int beforedp = 0;
	for(ptr = retstr; *ptr != '\0'; ptr++){
	if(*ptr == '.'){
	break;
	} else {
	beforedp++;
	}
	}
	fillzero = sv->length - beforedp + sv->power;
	for(zcount = 0; zcount < fillzero; zcount++){
	final[zcount] = '0';
	}
	memcpy(final + fillzero, retstr, beforedp);
	if(sv->power < 0){
	int afterdp = 0;
	if(*ptr != '\0'){
	ptr++;
	// after decimal point
	for(; *ptr != '\0'; ptr++){
	afterdp++;
	}
	// fill zero
	memcpy(final + fillzero + beforedp,
	retstr + beforedp + DECIMAL_LENGTH, afterdp);
	}
	fillzero = - sv->power - afterdp;
	for(zcount = 0; zcount < fillzero; zcount++){
	final[zcount] = '0';
	}
	}
	memcpy(addr, final, sv->length);
	free(final);
	break;
	}
	case OCDB_TYPE_SIGNED_NUMBER_TC:
	{
	char *value;
	char *ptr;
	int is_negative = false;
	int fillzero;
	int zcount;
	char *final;
	int finalbuflen;
	int final_length;
	// fill zero
	finalbuflen = sv->length;
	final = (char *)calloc(finalbuflen, sizeof(char));
	if(retstr[0] == '-'){
	is_negative = true;
	value = retstr + 1;
	} else {
	value = retstr;
	}
	// before decimal point
	int beforedp = 0;
	for(ptr = value; *ptr != '\0'; ptr++){
	if(*ptr == '.'){
	break;
	} else {
	beforedp++;
	}
	}
	fillzero = sv->length - beforedp + sv->power;
	for(zcount = 0; zcount < fillzero; zcount++){
	final[zcount] = '0';
	}
	memcpy(final + fillzero, value, beforedp);
	if(sv->power < 0){
	int afterdp = 0;
	if(*ptr != '\0'){
	ptr++;
	// after decimal point
	for(; *ptr != '\0'; ptr++){
	afterdp++;
	}
	memcpy(final + fillzero + beforedp, value +
	beforedp + DECIMAL_LENGTH, afterdp);
	}
	// fill zero
	fillzero = - sv->power - afterdp;
	for(zcount = 0; zcount < fillzero; zcount++){
	final[zcount] = '0';
	}
	}
	final_length = strlen(final);
	if(is_negative){
	int index = *(final + (final_length - 1)) - '0';
	final[final_length - 1] = type_tc_negative_final_number[index];
	}
	memcpy(addr, final, sv->length);
	free(final);
	break;
	}
	case OCDB_TYPE_SIGNED_NUMBER_LS:
	{
	char *value;
	char *ptr;
	int fillzero;
	int zcount;
	char *final;
	int finalbuflen;
	// fill zero
	finalbuflen = SIGN_LENGTH + sv->length + TERMINAL_LENGTH;
	final = (char *)calloc(finalbuflen, sizeof(char));
	if(retstr[0] == '-'){
	final[0] = '-';
	value = retstr + 1;
	} else {
	final[0] = '+';
	value = retstr;
	}
	// before decimal point
	int beforedp = 0;
	for(ptr = value; *ptr != '\0'; ptr++){
	if(*ptr == '.'){
	break;
	} else {
	beforedp++;
	}
	}
	fillzero = sv->length - beforedp + sv->power;
	for(zcount = 0; zcount < fillzero; zcount++){
	final[zcount] = '0';
	}
	memcpy(final + SIGN_LENGTH + fillzero, value, beforedp);
	if(sv->power < 0){
	int afterdp = 0;
	if(*ptr != '\0'){
	ptr++;
	// after decimal point
	for(; *ptr != '\0'; ptr++){
	afterdp++;
	}
	// fill zero
	memcpy(final + SIGN_LENGTH + fillzero + beforedp,
	value + beforedp + DECIMAL_LENGTH, afterdp);
	}
	fillzero = - sv->power - afterdp;
	for(zcount = 0; zcount < fillzero; zcount++){
	final[zcount] = '0';
	}
	}
	memcpy(addr, final, sv->length + SIGN_LENGTH);
	free(final);
	break;
	}
	case OCDB_TYPE_UNSIGNED_NUMBER_PD:
	{
	char *value = retstr;
	char *ptr;
	int is_negative = false;
	int fillzero;
	int zcount;
	char *pre_final;
	int pre_final_len;
	char *final;
	double dlength;
	int skip_first;
	int i;
	unsigned char ubit = 0xF0;
	unsigned char lbit = 0x0F;
	dlength = ceil(((double)sv->length + 1)/2);
	skip_first = (sv->length + 1) % 2; // 1 -> skip first 4 bits
	pre_final_len = sv->length + TERMINAL_LENGTH;
	pre_final = (char *)calloc(pre_final_len, sizeof(char));
	// before decimal point
	int beforedp = 0;
	for(ptr = value; *ptr != '\0'; ptr++){
	if(*ptr == '.'){
	break;
	} else {
	beforedp++;
	}
	}
	fillzero = sv->length - beforedp + sv->power;
	for(zcount = 0; zcount < fillzero; zcount++){
	pre_final[zcount] = '0';
	}
	memcpy(pre_final + fillzero, value, beforedp);
	if(sv->power < 0){
	int afterdp = 0;
	if(*ptr != '\0'){
	ptr++;
	// after decimal point
	for(; *ptr != '\0'; ptr++){
	afterdp++;
	}
	memcpy(pre_final + fillzero + beforedp,
	value + beforedp + DECIMAL_LENGTH, afterdp);
	}
	// fill zero
	fillzero = - sv->power - afterdp;
	for(zcount = 0; zcount < fillzero; zcount++){
	pre_final[zcount] = '0';
	}
	}
	// format setting
	final = (char *)calloc((int)dlength + TERMINAL_LENGTH, sizeof(char));
	ptr = pre_final;
	for(i=0; i<dlength; i++){
	unsigned char vubit = 0x00;
	unsigned char vlbit = 0x00;
	if(i == 0 && skip_first){
	vubit = 0x00;
	} else {
	vubit = (*ptr) << 4;
	vubit = vubit & ubit;
	ptr++;
	}
	if(i != dlength - 1){
	vlbit = *ptr;
	vlbit = vlbit & lbit;
	ptr++;
	} else {
	vlbit = 0x0F;
	}
	final[i] = vubit | vlbit;
	}
	memcpy(addr, final, (int)dlength);
	free(pre_final);
	free(final);
	break;
	}
	case OCDB_TYPE_SIGNED_NUMBER_PD:
	{
	char *value;
	char *ptr;
	int is_negative = false;
	int fillzero;
	int zcount;
	char *pre_final;
	int pre_final_len;
	char *final;
	double dlength;
	int skip_first;
	int i;
	unsigned char ubit = 0xF0;
	unsigned char lbit = 0x0F;
	dlength = ceil((double)(sv->length + 1)/2);
	skip_first = (sv->length + 1) % 2; // 1 -> skip first 4 bits
	if(retstr[0] == '-'){
	is_negative = true;
	value = retstr + 1;
	} else {
	value = retstr;
	}
	pre_final_len = (int)dlength + TERMINAL_LENGTH;
	pre_final = (char *)calloc(pre_final_len, sizeof(char));
	// before decimal point
	int beforedp = 0;
	for(ptr = value; *ptr != '\0'; ptr++){
	if(*ptr == '.'){
	break;
	} else {
	beforedp++;
	}
	}
	fillzero = sv->length - beforedp + sv->power;
	for(zcount = 0; zcount < fillzero; zcount++){
	pre_final[zcount] = '0';
	}
	memcpy(pre_final + fillzero, value, beforedp);
	if(sv->power < 0){
	int afterdp = 0;
	if(*ptr != '\0'){
	ptr++;
	// after decimal point
	for(; *ptr != '\0'; ptr++){
	afterdp++;
	}
	memcpy(pre_final + fillzero + beforedp,
	value + beforedp + DECIMAL_LENGTH, afterdp);
	}
	// fill zero
	fillzero = - sv->power - afterdp;
	for(zcount = 0; zcount < fillzero; zcount++){
	pre_final[zcount] = '0';
	}
	}
	// format setting
	final = (char *)calloc((int)dlength + TERMINAL_LENGTH, sizeof(char));
	ptr = pre_final;
	for(i=0; i<dlength; i++){
	unsigned char vubit = 0x00;
	unsigned char vlbit = 0x00;
	if(i == 0 && skip_first){
	vubit = 0x00;
	} else {
	vubit = (*ptr) << 4;
	vubit = vubit & ubit;
	ptr++;
	}
	if(i != dlength - 1){
	vlbit = *ptr;
	vlbit = vlbit & lbit;
	ptr++;
	} else {
	if(is_negative){
	vlbit = 0x0D;
	} else {
	vlbit = 0x0C;
	}
	}
	final[i] = vubit | vlbit;
	}
	memcpy(addr, final, (int)dlength);
	free(pre_final);
	free(final);
	break;
	}

• the driver queries the data, for example with an integer type in the DB you may get 2147483647
• it translates the data to its string value, so for our example "2147483647"
• the library frontend takes this value and sets the COBOL data

... so far: totally correct.

The issue is that in all those places the local allocated buffer that is used to write the data to is allocated based on dlength - the length of the COBOL field, but the memcpy is based on the strlen(data), which can be bigger.

In the example above, given a COBOL definition of S9(8) we allocate 8 (+1 for strlen) bytes, but create_coboldata copies the 10 bytes from the data size (*ptr != '\0' / strlen()). In the case of OCDB_TYPE_SIGNED_NUMBER_TC:

Open-COBOL-ESQL/dblib/ocesql.c

Lines 2867 to 2881 in 1b272b8

	// before decimal point
	int beforedp = 0;
	for(ptr = value; *ptr != '\0'; ptr++){
	if(*ptr == '.'){
	break;
	} else {
	beforedp++;
	}
	}
	fillzero = sv->length - beforedp + sv->power;
	for(zcount = 0; zcount < fillzero; zcount++){
	final[zcount] = '0';
	}
	memcpy(final + fillzero, value, beforedp);

we compute fillzero which will end up with -2 and then copy the data "two bytes before the allocated buffer".

If this does not crash the memory layout, then the end result looks correct (left-truncation).

As seen above the truncation must be explicit checked, both to return the correct values and even more important to don't trash the memory.

And very likely after the data is stored with COBOL correct truncation an sqlcode for "field truncated" should be set.

[GitMensch]

I'm going to send in a PR for both this issue and #31 in the next days.