Coverity reported buffer overrun error in "winebuild: Add support for 16-bit exe modules."
Coverity complains that http://source.winehq.org/git/wine.git/?a=commitdiff;h=0c214a7091af8efe39ffda... introduced a buffer overrun in winebuild. It looks like somebody forgot to dynamically grow an array?
Here's the report. Can somebody familiar with the code (or with a little time on their hands) have a look? Thanks... - Dan
CID: 682 Checker: OVERRUN_DYNAMIC (help) File: base/src/wine/tools/winebuild/spec16.c Function: BuildSpec16File Description: Overrun of dynamic array "(spec)->ordinals" of size 4 at position 4 with index variable "(i * 4)"
524 /******************************************************************* 525 * BuildSpec16File 526 * 527 * Build a Win16 assembly file from a spec file. 528 */ 529 void BuildSpec16File( DLLSPEC *spec ) 530 { 531 ORDDEF **typelist; 532 ORDDEF *entry_point = NULL; 533 int i, j, nb_funcs; 534 char header_name[256]; 535 536 /* File header */ 537 538 output_standard_file_header(); 539 540 if (!spec->file_name) 541 { 542 char *p; 543 spec->file_name = xstrdup( output_file_name ); 544 if ((p = strrchr( spec->file_name, '.' ))) *p = 0; 545 } 546 if (!spec->dll_name) /* set default name from file name */ 547 { 548 char *p; 549 spec->dll_name = xstrdup( spec->file_name ); 550 if ((p = strrchr( spec->dll_name, '.' ))) *p = 0; 551 } 552 553 /* store the main entry point as ordinal 0 */ 554 555 if (!spec->ordinals) 556 {
Event buffer_alloc: Called allocating function "xmalloc" which allocated 4 bytes dictated by parameter 0 [model] Event var_assign: Assigned "(spec)->ordinals" to storage allocated by "xmalloc" Also see events: [var_assign][overrun-local]
557 spec->ordinals = xmalloc( sizeof(spec->ordinals[0]) ); 558 spec->ordinals[0] = NULL; 559 }
At conditional (1): "(spec)->init_func != 0" taking true path At conditional (2): "(spec)->characteristics & 8192 == 0" taking true path
560 if (spec->init_func && !(spec->characteristics & IMAGE_FILE_DLL)) 561 { 562 entry_point = xmalloc( sizeof(*entry_point) ); 563 entry_point->type = TYPE_PASCAL; 564 entry_point->ordinal = 0; 565 entry_point->lineno = 0; 566 entry_point->flags = FLAG_REGISTER; 567 entry_point->name = NULL; 568 entry_point->link_name = xstrdup( spec->init_func ); 569 entry_point->export_name = NULL; 570 entry_point->u.func.arg_types[0] = 0;
At conditional (3): "*((spec)->ordinals + 0) == 0" taking true path
571 assert( !spec->ordinals[0] ); 572 spec->ordinals[0] = entry_point; 573 } 574 575 /* Build sorted list of all argument types, without duplicates */ 576 577 typelist = xmalloc( (spec->limit + 1) * sizeof(*typelist) ); 578
At conditional (4): "i <= (spec)->limit" taking true path At conditional (6): "i <= (spec)->limit" taking true path At conditional (8): "i <= (spec)->limit" taking true path At conditional (10): "i <= (spec)->limit" taking true path At conditional (13): "i <= (spec)->limit" taking true path At conditional (16): "i <= (spec)->limit" taking false path
579 for (i = nb_funcs = 0; i <= spec->limit; i++) 580 { 581 ORDDEF *odp = spec->ordinals[i];
At conditional (5): "odp == 0" taking true path At conditional (7): "odp == 0" taking true path At conditional (9): "odp == 0" taking true path At conditional (11): "odp == 0" taking false path At conditional (14): "odp == 0" taking false path
582 if (!odp) continue;
At conditional (12): "is_function != 0" taking false path At conditional (15): "is_function != 0" taking true path
583 if (is_function( odp )) typelist[nb_funcs++] = odp; 584 } 585 586 nb_funcs = sort_func_list( typelist, nb_funcs, callfrom16_type_compare ); 587 588 /* Output the module structure */ 589 590 sprintf( header_name, "__wine_spec_%s_dos_header", make_c_identifier(spec->dll_name) ); 591 output( "\n/* module data */\n\n" ); 592 output( "\t.data\n" ); 593 output( "\t.align %d\n", get_alignment(4) ); 594 output( "%s:\n", header_name ); 595 output( "\t%s 0x%04x\n", get_asm_short_keyword(), /* e_magic */ 596 IMAGE_DOS_SIGNATURE ); 597 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cblp */ 598 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cp */ 599 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_crlc */ 600 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cparhdr */ 601 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_minalloc */ 602 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_maxalloc */ 603 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_ss */ 604 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_sp */ 605 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_csum */ 606 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_ip */ 607 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cs */ 608 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_lfarlc */ 609 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_ovno */ 610 output( "\t%s 0,0,0,0\n", get_asm_short_keyword() ); /* e_res */ 611 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_oemid */ 612 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_oeminfo */ 613 output( "\t%s 0,0,0,0,0,0,0,0,0,0\n", get_asm_short_keyword() ); /* e_res2 */ 614 output( "\t.long .L__wine_spec_ne_header-%s\n", header_name ); /* e_lfanew */ 615 616 output( ".L__wine_spec_ne_header:\n" ); 617 output( "\t%s 0x%04x\n", get_asm_short_keyword(), /* ne_magic */ 618 IMAGE_OS2_SIGNATURE ); 619 output( "\t.byte 0\n" ); /* ne_ver */ 620 output( "\t.byte 0\n" ); /* ne_rev */ 621 output( "\t%s .L__wine_spec_ne_enttab-.L__wine_spec_ne_header\n", /* ne_enttab */ 622 get_asm_short_keyword() ); 623 output( "\t%s .L__wine_spec_ne_enttab_end-.L__wine_spec_ne_enttab\n", /* ne_cbenttab */ 624 get_asm_short_keyword() ); 625 output( "\t.long 0\n" ); /* ne_crc */
At conditional (17): "(spec)->characteristics & 8192 != 0" taking false path
626 output( "\t%s 0x%04x\n", get_asm_short_keyword(), /* ne_flags */ 627 NE_FFLAGS_SINGLEDATA | 628 ((spec->characteristics & IMAGE_FILE_DLL) ? NE_FFLAGS_LIBMODULE : 0) ); 629 output( "\t%s 2\n", get_asm_short_keyword() ); /* ne_autodata */ 630 output( "\t%s %u\n", get_asm_short_keyword(), spec->heap_size ); /* ne_heap */ 631 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_stack */
At conditional (18): "entry_point == 0" taking false path
632 if (!entry_point) output( "\t.long 0\n" ); /* ne_csip */ 633 else output( "\t%s .L__wine_%s_0-.L__wine_spec_code_segment,1\n", 634 get_asm_short_keyword(), make_c_identifier(spec->dll_name) ); 635 output( "\t%s 0,2\n", get_asm_short_keyword() ); /* ne_sssp */ 636 output( "\t%s 2\n", get_asm_short_keyword() ); /* ne_cseg */ 637 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cmod */ 638 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cbnrestab */ 639 output( "\t%s .L__wine_spec_ne_segtab-.L__wine_spec_ne_header\n", /* ne_segtab */ 640 get_asm_short_keyword() ); 641 output( "\t%s .L__wine_spec_ne_rsrctab-.L__wine_spec_ne_header\n", /* ne_rsrctab */ 642 get_asm_short_keyword() ); 643 output( "\t%s .L__wine_spec_ne_restab-.L__wine_spec_ne_header\n", /* ne_restab */ 644 get_asm_short_keyword() ); 645 output( "\t%s .L__wine_spec_ne_modtab-.L__wine_spec_ne_header\n", /* ne_modtab */ 646 get_asm_short_keyword() ); 647 output( "\t%s .L__wine_spec_ne_imptab-.L__wine_spec_ne_header\n", /* ne_imptab */ 648 get_asm_short_keyword() ); 649 output( "\t.long 0\n" ); /* ne_nrestab */ 650 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cmovent */ 651 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_align */ 652 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cres */ 653 output( "\t.byte 0x%02x\n", NE_OSFLAGS_WINDOWS ); /* ne_exetyp */ 654 output( "\t.byte 0x%02x\n", NE_AFLAGS_FASTLOAD ); /* ne_flagsothers */ 655 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_pretthunks */ 656 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_psegrefbytes */ 657 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_swaparea */ 658 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_expver */ 659 660 /* segment table */ 661 662 output( "\n.L__wine_spec_ne_segtab:\n" ); 663 664 /* code segment entry */ 665 666 output( "\t%s .L__wine_spec_code_segment-%s\n", /* filepos */ 667 get_asm_short_keyword(), header_name ); 668 output( "\t%s .L__wine_spec_code_segment_end-.L__wine_spec_code_segment\n", /* size */ 669 get_asm_short_keyword() ); 670 output( "\t%s 0x%04x\n", get_asm_short_keyword(), NE_SEGFLAGS_32BIT ); /* flags */ 671 output( "\t%s .L__wine_spec_code_segment_end-.L__wine_spec_code_segment\n", /* minsize */ 672 get_asm_short_keyword() ); 673 674 /* data segment entry */ 675 676 output( "\t%s .L__wine_spec_data_segment-%s\n", /* filepos */ 677 get_asm_short_keyword(), header_name ); 678 output( "\t%s .L__wine_spec_data_segment_end-.L__wine_spec_data_segment\n", /* size */ 679 get_asm_short_keyword() ); 680 output( "\t%s 0x%04x\n", get_asm_short_keyword(), NE_SEGFLAGS_DATA ); /* flags */ 681 output( "\t%s .L__wine_spec_data_segment_end-.L__wine_spec_data_segment\n", /* minsize */ 682 get_asm_short_keyword() ); 683 684 /* resource directory */ 685 686 output_res16_directory( spec, header_name ); 687 688 /* resident names table */ 689 690 output( "\n\t.align %d\n", get_alignment(2) ); 691 output( ".L__wine_spec_ne_restab:\n" ); 692 output_resident_name( spec->dll_name, 0 );
At conditional (19): "i <= (spec)->limit" taking true path
693 for (i = 1; i <= spec->limit; i++) 694 {
Event overrun-local: Overrun of dynamic array "(spec)->ordinals" of size 4 at position 4 with index variable "(i * 4)" Also see events: [buffer_alloc][var_assign]
695 ORDDEF *odp = spec->ordinals[i];
On Sat, Jun 28, 2008 at 07:17:42AM -0700, Dan Kegel wrote:
Coverity complains that http://source.winehq.org/git/wine.git/?a=commitdiff;h=0c214a7091af8efe39ffda... introduced a buffer overrun in winebuild. It looks like somebody forgot to dynamically grow an array?
Here's the report. Can somebody familiar with the code (or with a little time on their hands) have a look? Thanks...
- Dan
I thought it to be a mixup between EXE and DLL generation there, but I did not understand it either. (that the EXE generator has the ordinals[1] array)
Ciao, Marcus
CID: 682 Checker: OVERRUN_DYNAMIC (help) File: base/src/wine/tools/winebuild/spec16.c Function: BuildSpec16File Description: Overrun of dynamic array "(spec)->ordinals" of size 4 at position 4 with index variable "(i * 4)"
524 /******************************************************************* 525 * BuildSpec16File 526 * 527 * Build a Win16 assembly file from a spec file. 528 */ 529 void BuildSpec16File( DLLSPEC *spec ) 530 { 531 ORDDEF **typelist; 532 ORDDEF *entry_point = NULL; 533 int i, j, nb_funcs; 534 char header_name[256]; 535 536 /* File header */ 537 538 output_standard_file_header(); 539 540 if (!spec->file_name) 541 { 542 char *p; 543 spec->file_name = xstrdup( output_file_name ); 544 if ((p = strrchr( spec->file_name, '.' ))) *p = 0; 545 } 546 if (!spec->dll_name) /* set default name from file name */ 547 { 548 char *p; 549 spec->dll_name = xstrdup( spec->file_name ); 550 if ((p = strrchr( spec->dll_name, '.' ))) *p = 0; 551 } 552 553 /* store the main entry point as ordinal 0 */ 554 555 if (!spec->ordinals) 556 {
Event buffer_alloc: Called allocating function "xmalloc" which allocated 4 bytes dictated by parameter 0 [model] Event var_assign: Assigned "(spec)->ordinals" to storage allocated by "xmalloc" Also see events: [var_assign][overrun-local]
557 spec->ordinals = xmalloc( sizeof(spec->ordinals[0]) ); 558 spec->ordinals[0] = NULL; 559 }
At conditional (1): "(spec)->init_func != 0" taking true path At conditional (2): "(spec)->characteristics & 8192 == 0" taking true path
560 if (spec->init_func && !(spec->characteristics & IMAGE_FILE_DLL)) 561 { 562 entry_point = xmalloc( sizeof(*entry_point) ); 563 entry_point->type = TYPE_PASCAL; 564 entry_point->ordinal = 0; 565 entry_point->lineno = 0; 566 entry_point->flags = FLAG_REGISTER; 567 entry_point->name = NULL; 568 entry_point->link_name = xstrdup( spec->init_func ); 569 entry_point->export_name = NULL; 570 entry_point->u.func.arg_types[0] = 0;
At conditional (3): "*((spec)->ordinals + 0) == 0" taking true path
571 assert( !spec->ordinals[0] ); 572 spec->ordinals[0] = entry_point; 573 } 574 575 /* Build sorted list of all argument types, without duplicates */ 576 577 typelist = xmalloc( (spec->limit + 1) * sizeof(*typelist) ); 578
At conditional (4): "i <= (spec)->limit" taking true path At conditional (6): "i <= (spec)->limit" taking true path At conditional (8): "i <= (spec)->limit" taking true path At conditional (10): "i <= (spec)->limit" taking true path At conditional (13): "i <= (spec)->limit" taking true path At conditional (16): "i <= (spec)->limit" taking false path
579 for (i = nb_funcs = 0; i <= spec->limit; i++) 580 { 581 ORDDEF *odp = spec->ordinals[i];
At conditional (5): "odp == 0" taking true path At conditional (7): "odp == 0" taking true path At conditional (9): "odp == 0" taking true path At conditional (11): "odp == 0" taking false path At conditional (14): "odp == 0" taking false path
582 if (!odp) continue;
At conditional (12): "is_function != 0" taking false path At conditional (15): "is_function != 0" taking true path
583 if (is_function( odp )) typelist[nb_funcs++] = odp; 584 } 585 586 nb_funcs = sort_func_list( typelist, nb_funcs, callfrom16_type_compare ); 587 588 /* Output the module structure */ 589 590 sprintf( header_name, "__wine_spec_%s_dos_header", make_c_identifier(spec->dll_name) ); 591 output( "\n/* module data */\n\n" ); 592 output( "\t.data\n" ); 593 output( "\t.align %d\n", get_alignment(4) ); 594 output( "%s:\n", header_name ); 595 output( "\t%s 0x%04x\n", get_asm_short_keyword(), /* e_magic */ 596 IMAGE_DOS_SIGNATURE ); 597 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cblp */ 598 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cp */ 599 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_crlc */ 600 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cparhdr */ 601 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_minalloc */ 602 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_maxalloc */ 603 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_ss */ 604 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_sp */ 605 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_csum */ 606 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_ip */ 607 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_cs */ 608 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_lfarlc */ 609 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_ovno */ 610 output( "\t%s 0,0,0,0\n", get_asm_short_keyword() ); /* e_res */ 611 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_oemid */ 612 output( "\t%s 0\n", get_asm_short_keyword() ); /* e_oeminfo */ 613 output( "\t%s 0,0,0,0,0,0,0,0,0,0\n", get_asm_short_keyword() ); /* e_res2 */ 614 output( "\t.long .L__wine_spec_ne_header-%s\n", header_name ); /* e_lfanew */ 615 616 output( ".L__wine_spec_ne_header:\n" ); 617 output( "\t%s 0x%04x\n", get_asm_short_keyword(), /* ne_magic */ 618 IMAGE_OS2_SIGNATURE ); 619 output( "\t.byte 0\n" ); /* ne_ver */ 620 output( "\t.byte 0\n" ); /* ne_rev */ 621 output( "\t%s .L__wine_spec_ne_enttab-.L__wine_spec_ne_header\n", /* ne_enttab */ 622 get_asm_short_keyword() ); 623 output( "\t%s .L__wine_spec_ne_enttab_end-.L__wine_spec_ne_enttab\n", /* ne_cbenttab */ 624 get_asm_short_keyword() ); 625 output( "\t.long 0\n" ); /* ne_crc */
At conditional (17): "(spec)->characteristics & 8192 != 0" taking false path
626 output( "\t%s 0x%04x\n", get_asm_short_keyword(), /* ne_flags */ 627 NE_FFLAGS_SINGLEDATA | 628 ((spec->characteristics & IMAGE_FILE_DLL) ? NE_FFLAGS_LIBMODULE : 0) ); 629 output( "\t%s 2\n", get_asm_short_keyword() ); /* ne_autodata */ 630 output( "\t%s %u\n", get_asm_short_keyword(), spec->heap_size ); /* ne_heap */ 631 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_stack */
At conditional (18): "entry_point == 0" taking false path
632 if (!entry_point) output( "\t.long 0\n" ); /* ne_csip */ 633 else output( "\t%s .L__wine_%s_0-.L__wine_spec_code_segment,1\n", 634 get_asm_short_keyword(), make_c_identifier(spec->dll_name) ); 635 output( "\t%s 0,2\n", get_asm_short_keyword() ); /* ne_sssp */ 636 output( "\t%s 2\n", get_asm_short_keyword() ); /* ne_cseg */ 637 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cmod */ 638 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cbnrestab */ 639 output( "\t%s .L__wine_spec_ne_segtab-.L__wine_spec_ne_header\n", /* ne_segtab */ 640 get_asm_short_keyword() ); 641 output( "\t%s .L__wine_spec_ne_rsrctab-.L__wine_spec_ne_header\n", /* ne_rsrctab */ 642 get_asm_short_keyword() ); 643 output( "\t%s .L__wine_spec_ne_restab-.L__wine_spec_ne_header\n", /* ne_restab */ 644 get_asm_short_keyword() ); 645 output( "\t%s .L__wine_spec_ne_modtab-.L__wine_spec_ne_header\n", /* ne_modtab */ 646 get_asm_short_keyword() ); 647 output( "\t%s .L__wine_spec_ne_imptab-.L__wine_spec_ne_header\n", /* ne_imptab */ 648 get_asm_short_keyword() ); 649 output( "\t.long 0\n" ); /* ne_nrestab */ 650 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cmovent */ 651 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_align */ 652 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_cres */ 653 output( "\t.byte 0x%02x\n", NE_OSFLAGS_WINDOWS ); /* ne_exetyp */ 654 output( "\t.byte 0x%02x\n", NE_AFLAGS_FASTLOAD ); /* ne_flagsothers */ 655 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_pretthunks */ 656 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_psegrefbytes */ 657 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_swaparea */ 658 output( "\t%s 0\n", get_asm_short_keyword() ); /* ne_expver */ 659 660 /* segment table */ 661 662 output( "\n.L__wine_spec_ne_segtab:\n" ); 663 664 /* code segment entry */ 665 666 output( "\t%s .L__wine_spec_code_segment-%s\n", /* filepos */ 667 get_asm_short_keyword(), header_name ); 668 output( "\t%s .L__wine_spec_code_segment_end-.L__wine_spec_code_segment\n", /* size */ 669 get_asm_short_keyword() ); 670 output( "\t%s 0x%04x\n", get_asm_short_keyword(), NE_SEGFLAGS_32BIT ); /* flags */ 671 output( "\t%s .L__wine_spec_code_segment_end-.L__wine_spec_code_segment\n", /* minsize */ 672 get_asm_short_keyword() ); 673 674 /* data segment entry */ 675 676 output( "\t%s .L__wine_spec_data_segment-%s\n", /* filepos */ 677 get_asm_short_keyword(), header_name ); 678 output( "\t%s .L__wine_spec_data_segment_end-.L__wine_spec_data_segment\n", /* size */ 679 get_asm_short_keyword() ); 680 output( "\t%s 0x%04x\n", get_asm_short_keyword(), NE_SEGFLAGS_DATA ); /* flags */ 681 output( "\t%s .L__wine_spec_data_segment_end-.L__wine_spec_data_segment\n", /* minsize */ 682 get_asm_short_keyword() ); 683 684 /* resource directory */ 685 686 output_res16_directory( spec, header_name ); 687 688 /* resident names table */ 689 690 output( "\n\t.align %d\n", get_alignment(2) ); 691 output( ".L__wine_spec_ne_restab:\n" ); 692 output_resident_name( spec->dll_name, 0 );
At conditional (19): "i <= (spec)->limit" taking true path
693 for (i = 1; i <= spec->limit; i++) 694 {
Event overrun-local: Overrun of dynamic array "(spec)->ordinals" of size 4 at position 4 with index variable "(i * 4)" Also see events: [buffer_alloc][var_assign]
695 ORDDEF *odp = spec->ordinals[i];
"Dan Kegel" dank@kegel.com writes:
Coverity complains that http://source.winehq.org/git/wine.git/?a=commitdiff;h=0c214a7091af8efe39ffda... introduced a buffer overrun in winebuild. It looks like somebody forgot to dynamically grow an array?
Here's the report. Can somebody familiar with the code (or with a little time on their hands) have a look?
The code is correct, spec->limit will always be 0 if there are no exported entry points, so the loop where Coverity sees an overflow is not actually executed.
-
Alexandre Julliard -
Dan Kegel -
Marcus Meissner