2018-06-25 12:25 EDT


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0002776FSSCPBuild systempublic2013-02-16 05:43
Reporteronlyjob 
Assigned Toniffiwan 
PrioritylowSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
Platformx86_64OSDebianOS Version7
Product Version3.6.14 
Target VersionFixed in Version 
Summary0002776: FTBFS with format-hardening [patch]
DescriptionPatch to fix FTBFS:
    "format not a string literal"
    "no format arguments [-Werror=format-security]"
TagsNo tags attached.
Attached Files
  • patch file icon hardening-format.patch (11,851 bytes) 2013-01-10 00:26 -
    Last-Update: 2012-12-27
    Author: Dmitry Smirnov <onlyjob@member.fsf.org>
    Forwarded: 2013-01-10
    Description: fixes FTBFS with format hardening
     Workaround for error: "format not a string literal and
     "no format arguments [-Werror=format-security]"
    
    --- a/code/cutscene/cutscenes.cpp
    +++ b/code/cutscene/cutscenes.cpp
    @@ -319,11 +319,11 @@
     	if ( !rval ) {
     		char str[256];
     
     		if (Cmdline_nomovies)
    -			sprintf(str, XSTR("Movies are currently disabled.", -1));
    +			sprintf(str, "%s", XSTR("Movies are currently disabled.", -1));
     		else
    -			sprintf(str, XSTR("Unable to play movie %s.", 204), Cutscenes[which_cutscene].name);
    +			sprintf(str, "%s", XSTR("Unable to play movie %s.", 204), Cutscenes[which_cutscene].name);
     
     		popup(0, 1, POPUP_OK, str );
     	}
     	
    --- a/code/gamehelp/gameplayhelp.cpp
    +++ b/code/gamehelp/gameplayhelp.cpp
    @@ -119,9 +119,9 @@
     
     	ci = &Control_config[id];
     
     	if ( ci->key_id >= 0 ) {
    -		sprintf(buf, textify_scancode(ci->key_id));
    +		sprintf(buf, "%s", textify_scancode(ci->key_id));
     		has_key=1;
     	}
     
     	if ( ci->joy_id >= 0 ) {
    @@ -152,9 +152,9 @@
     
     	buf[0] = 0;
     
     	if ( ci->key_id >= 0 ) {
    -		sprintf(buf, textify_scancode(ci->key_id));
    +		sprintf(buf, "%s", textify_scancode(ci->key_id));
     		has_key=1;
     	}
     
     	if ( ci->joy_id >= 0 ) {
    --- a/code/gamesnd/eventmusic.cpp
    +++ b/code/gamesnd/eventmusic.cpp
    @@ -1685,12 +1685,12 @@
     // NOTE: callers to this function are advised to allocate a 256 byte buffer
     void event_music_get_info(char *outbuf)
     {
     	if ( Event_music_enabled == FALSE || Event_music_level_inited == FALSE || Current_pattern == -1 ) {
    -		sprintf(outbuf,XSTR( "Event music is not playing", 213));
    +		sprintf(outbuf, "%s", XSTR( "Event music is not playing", 213));
     	}
     	else {	
    -		sprintf(outbuf,XSTR( "soundtrack: %s [%s]", 214), Soundtracks[Current_soundtrack_num].name, Pattern_info[Current_pattern].pattern_desc);
    +		sprintf(outbuf, "%s", XSTR( "soundtrack: %s [%s]", 214), Soundtracks[Current_soundtrack_num].name, Pattern_info[Current_pattern].pattern_desc);
     	}
     }
     
     // ----------------------------------------------------------------
    @@ -1751,9 +1751,9 @@
     	if ( Event_music_enabled == FALSE || Event_music_level_inited == FALSE ) {
     		strcpy(outbuf, XSTR( "Event music is not playing", 213));
     	}
     	else {
    -		sprintf(outbuf, Soundtracks[Current_soundtrack_num].name);
    +		sprintf(outbuf, "%s", Soundtracks[Current_soundtrack_num].name);
     	}
     }
     
     // set the current soundtrack based on name
    --- a/code/hud/hud.cpp
    +++ b/code/hud/hud.cpp
    @@ -2807,11 +2807,11 @@
     				}
     			}
     
     			if (repairing)
    -				sprintf(outstr, XSTR("repairing", 227));
    +				sprintf(outstr, "%s", XSTR("repairing", 227));
     			else
    -				sprintf(outstr, XSTR("rearming", 228));
    +				sprintf(outstr, "%s", XSTR("rearming", 228));
     		}
     		else
     		{
     			if (Player_rearm_eta > 0)
    @@ -2831,32 +2831,32 @@
     		}
     		renderStringAlignCenter(position[0], position[1] + text_val_offset_y, w, outstr);
     	}
     	else if (Player_ai->ai_flags & AIF_REPAIR_OBSTRUCTED) {
    -		sprintf(outstr, XSTR( "obstructed", 229));
    +		sprintf(outstr, "%s", XSTR( "obstructed", 229));
     		renderStringAlignCenter(position[0], position[1] + text_val_offset_y, w, outstr);
     	} else {
     		if ( Hud_support_objnum == -1 ) {
     			if (The_mission.support_ships.arrival_location == ARRIVE_FROM_DOCK_BAY)
     			{
    -				sprintf(outstr, XSTR( "exiting hangar", -1));
    +				sprintf(outstr, "%s", XSTR( "exiting hangar", -1));
     			}
     			else
     			{
    -				sprintf(outstr, XSTR( "warping in", 230));
    +				sprintf(outstr, "%s", XSTR( "warping in", 230));
     			}
     			renderStringAlignCenter(position[0], position[1] + text_val_offset_y, w, outstr);
     		} else {
     			ai_info *aip;
     
     			// Display "busy" when support ship isn't actually enroute to me
     			aip = &Ai_info[Ships[Objects[Hud_support_objnum].instance].ai_index];
     			if ( aip->goal_objnum != OBJ_INDEX(Player_obj) ) {
    -				sprintf(outstr, XSTR( "busy", 231));
    +				sprintf(outstr, "%s", XSTR( "busy", 231));
     				show_time = 0;
     
     			} else {
    -				sprintf(outstr, XSTR( "dock in:", 232));
    +				sprintf(outstr, "%s", XSTR( "dock in:", 232));
     				show_time = 1;
     			}		
     
     			renderString(position[0] + text_dock_offset_x, position[1] + text_val_offset_y, outstr);
    @@ -3815,11 +3815,11 @@
     		// If our ping is positive, display it
     		if((Netgame.server != NULL) && (Netgame.server->s_info.ping.ping_avg > 0)){
     			// Get the string
     			if(Netgame.server->s_info.ping.ping_avg >= 1000){
    -				sprintf(ping_str,XSTR("> 1 sec",628));
    +				sprintf(ping_str, "%s", XSTR("> 1 sec",628));
     			} else {
    -				sprintf(ping_str,XSTR("%d ms",629),Netgame.server->s_info.ping.ping_avg);
    +				sprintf(ping_str, "%s", XSTR("%d ms",629),Netgame.server->s_info.ping.ping_avg);
     			}
     
     			// Blit the string out
     			hud_set_default_color();
    --- a/code/hud/hudtargetbox.cpp
    +++ b/code/hud/hudtargetbox.cpp
    @@ -388,18 +388,18 @@
     
     	// print out status of ship
     	switch(Current_ts) {
     		case TS_DIS:
    -			sprintf(buf,XSTR( "dis", 344));
    +			sprintf(buf, "%s", XSTR( "dis", 344));
     			break;
     		case TS_OK:
    -			sprintf(buf,XSTR( "ok", 345));
    +			sprintf(buf, "%s", XSTR( "ok", 345));
     			break;
     		case TS_DMG:
    -			sprintf(buf,XSTR( "dmg", 346));
    +			sprintf(buf, "%s", XSTR( "dmg", 346));
     			break;
     		case TS_CRT:
    -			sprintf(buf,XSTR( "crt", 347));
    +			sprintf(buf, "%s", XSTR( "crt", 347));
     			break;
     	}
     
     	maybeFlashElement(TBOX_FLASH_STATUS);
    @@ -739,11 +739,11 @@
     
     		dist = vm_vec_dist(&target_objp->pos, &wp->homing_object->pos);
     		speed = vm_vec_mag(&target_objp->phys_info.vel);
     		if ( speed > 0 ) {
    -			sprintf(outstr, NOX("impact: %.1f sec"), dist/speed);
    +			sprintf(outstr, "%s", NOX("impact: %.1f sec"), dist/speed);
     		} else {
    -			sprintf(outstr, XSTR( "unknown", 349));
    +			sprintf(outstr, "%s", XSTR( "unknown", 349));
     		}
     
     		renderString(position[0] + Class_offsets[0], position[1] + Class_offsets[1], EG_TBOX_CLASS, outstr);		
     	}
    @@ -1084,9 +1084,9 @@
     			renderString(position[0] + order_offsets[0], position[1] + order_offsets[1], EG_TBOX_EXTRA1, outstr);			
     		}
     
     		if ( has_orders ) {
    -			sprintf(outstr, XSTR( "time to: ", 338));
    +			sprintf(outstr, "%s", XSTR( "time to: ", 338));
     			if ( ship_return_time_to_goal(tmpbuf, target_shipp) ) {
     				strcat_s(outstr, tmpbuf);
     				
     				renderString(position[0] + time_offsets[0], position[1] + time_offsets[1], EG_TBOX_EXTRA2, outstr);				
    @@ -1369,11 +1369,11 @@
     
     	// print out 'disabled' on the monitor if the target is disabled
     	if ( (target_shipp->flags & SF_DISABLED) || (ship_subsys_disrupted(target_shipp, SUBSYSTEM_ENGINE)) ) {
     		if ( target_shipp->flags & SF_DISABLED ) {
    -			sprintf(outstr, XSTR( "DISABLED", 342));
    +			sprintf(outstr, "%s", XSTR( "DISABLED", 342));
     		} else {
    -			sprintf(outstr, XSTR( "DISRUPTED", 343));
    +			sprintf(outstr, "%s", XSTR( "DISRUPTED", 343));
     		}
     		gr_get_string_size(&w,&h,outstr);
     		renderPrintf(position[0] + Viewport_offsets[0] + Viewport_w/2 - w/2 - 1, position[1] + Viewport_offsets[1] + Viewport_h - 2*h, "%s", outstr);
     	}
    --- a/code/network/chat_api.cpp
    +++ b/code/network/chat_api.cpp
    @@ -1107,9 +1107,9 @@
     	}
     	if(stricmp(szCmd,"432")==0)
     	{
     		//Channel Mode info
    -		snprintf(szResponse, SSIZE(szResponse), XSTR("Your nickname contains invalid characters", 640));
    +		snprintf(szResponse, SSIZE(szResponse), "%s", XSTR("Your nickname contains invalid characters", 640));
     		AddChatCommandToQueue(CC_DISCONNECTED,NULL,0);
     		return szResponse;
     	}
     	if(stricmp(szCmd,"433")==0)
    --- a/code/object/objectsnd.cpp
    +++ b/code/object/objectsnd.cpp
    @@ -121,15 +121,15 @@
     					sprintf(buf1,"ON");
     				}
     
     				if ( Objects[osp->objnum].type == OBJ_SHIP ) {
    -					sprintf(buf2, Ships[Objects[osp->objnum].instance].ship_name);
    +					sprintf(buf2, "%s", Ships[Objects[osp->objnum].instance].ship_name);
     				}
     				else if ( Objects[osp->objnum].type == OBJ_DEBRIS ) {
    -					sprintf(buf2, "Debris");
    +					sprintf(buf2, "%s", "Debris");
     				}
     				else {
    -					sprintf(buf2, "Unknown");
    +					sprintf(buf2, "%s", "Unknown");
     				}
     
     				vec3d source_pos;
     				float distance;
    --- a/code/playerman/playercontrol.cpp
    +++ b/code/playerman/playercontrol.cpp
    @@ -1445,9 +1445,9 @@
     			} else {
     				sprintf(outstr,XSTR("cargo: %s", 84), cargo_name );
     			}
     		} else {
    -			sprintf(outstr, XSTR( "Scanned", 85) );
    +			sprintf(outstr, "%s", XSTR( "Scanned", 85) );
     		}
     
     		// always bash cargo_inspect_time to 0 since AI ships can reveal cargo that we
     		// are in the process of scanning
    @@ -1463,11 +1463,11 @@
     		vm_vec_normalized_dir(&vec_to_cargo, &cargo_objp->pos, &Player_obj->pos);
     		dot = vm_vec_dot(&vec_to_cargo, &Player_obj->orient.vec.fvec);
     		if ( dot < CARGO_MIN_DOT_TO_REVEAL ) {
     			if ( !(cargo_sp->flags & SF_SCANNABLE) )
    -				sprintf(outstr,XSTR( "cargo: <unknown>", 86));
    +				sprintf(outstr, "%s", XSTR( "cargo: <unknown>", 86));
     			else
    -				sprintf(outstr,XSTR( "not scanned", 87));
    +				sprintf(outstr, "%s", XSTR( "not scanned", 87));
     			hud_targetbox_end_flash(TBOX_FLASH_CARGO);
     			Player->cargo_inspect_time = 0;
     			return 1;
     		}
    @@ -1477,22 +1477,22 @@
     			Player->cargo_inspect_time += fl2i(frametime*1000+0.5f);
     		}
     
     		if ( !(cargo_sp->flags & SF_SCANNABLE) )
    -			sprintf(outstr,XSTR( "cargo: inspecting", 88));
    +			sprintf(outstr, "%s", XSTR( "cargo: inspecting", 88));
     		else
    -			sprintf(outstr,XSTR( "scanning", 89));
    +			sprintf(outstr, "%s", XSTR( "scanning", 89));
     
     		if ( Player->cargo_inspect_time > cargo_sip->scan_time ) {
     			ship_do_cargo_revealed( cargo_sp );
     			snd_play( &Snds[SND_CARGO_REVEAL], 0.0f );
     			Player->cargo_inspect_time = 0;
     		}
     	} else {
     		if ( !(cargo_sp->flags & SF_SCANNABLE) )
    -			sprintf(outstr,XSTR( "cargo: <unknown>", 86));
    +			sprintf(outstr, "%s", XSTR( "cargo: <unknown>", 86));
     		else
    -			sprintf(outstr,XSTR( "not scanned", 87));
    +			sprintf(outstr, "%s", XSTR( "not scanned", 87));
     	}
     
     	return 1;
     }
    @@ -1542,9 +1542,9 @@
     			} else {
     				sprintf(outstr,XSTR("cargo: %s", 84), cargo_name );
     			}
     		} else {
    -			sprintf(outstr, XSTR( "Scanned", 85) );
    +			sprintf(outstr, "%s", XSTR( "Scanned", 85) );
     		}
     	
     		// always bash cargo_inspect_time to 0 since AI ships can reveal cargo that we
     		// are in the process of scanning
    @@ -1578,11 +1578,11 @@
     		subsys_in_view = hud_targetbox_subsystem_in_view(cargo_objp, &x, &y);
     
     		if ( (dot < CARGO_MIN_DOT_TO_REVEAL) || (!subsys_in_view) ) {
     			if ( !(cargo_sp->flags & SF_SCANNABLE) )
    -				sprintf(outstr,XSTR( "cargo: <unknown>", 86));
    +				sprintf(outstr, "%s", XSTR( "cargo: <unknown>", 86));
     			else
    -				sprintf(outstr,XSTR( "not scanned", 87));
    +				sprintf(outstr, "%s", XSTR( "not scanned", 87));
     			hud_targetbox_end_flash(TBOX_FLASH_CARGO);
     			Player->cargo_inspect_time = 0;
     			return 1;
     		}
    @@ -1592,22 +1592,22 @@
     			Player->cargo_inspect_time += fl2i(frametime*1000+0.5f);
     		}
     
     		if ( !(cargo_sp->flags & SF_SCANNABLE) )
    -			sprintf(outstr,XSTR( "cargo: inspecting", 88));
    +			sprintf(outstr, "%s", XSTR( "cargo: inspecting", 88));
     		else
    -			sprintf(outstr,XSTR( "scanning", 89));
    +			sprintf(outstr, "%s", XSTR( "scanning", 89));
     
     		if ( Player->cargo_inspect_time > cargo_sip->scan_time ) {
     			ship_do_cap_subsys_cargo_revealed( cargo_sp, subsys, 0);
     			snd_play( &Snds[SND_CARGO_REVEAL], 0.0f );
     			Player->cargo_inspect_time = 0;
     		}
     	} else {
     		if ( !(cargo_sp->flags & SF_SCANNABLE) )
    -			sprintf(outstr,XSTR( "cargo: <unknown>", 86));
    +			sprintf(outstr, "%s", XSTR( "cargo: <unknown>", 86));
     		else
    -			sprintf(outstr,XSTR( "not scanned", 87));
    +			sprintf(outstr, "%s", XSTR( "not scanned", 87));
     	}
     
     	return 1;
     }
    --- a/code/ship/ship.cpp
    +++ b/code/ship/ship.cpp
    @@ -14007,9 +14007,9 @@
     			seconds = 99;
     		}
     		sprintf(outbuf, NOX("%02d:%02d"), minutes, seconds);
     	} else {
    -		sprintf( outbuf, XSTR( "Unknown", 497) );
    +		sprintf( outbuf, "%s", XSTR( "Unknown", 497) );
     	}
     
     	return outbuf;
     }
    
    patch file icon hardening-format.patch (11,851 bytes) 2013-01-10 00:26 +
  • patch file icon hardening-format[trunk].patch (769 bytes) 2013-01-13 03:15 -
    Last-Update: 2013-01-13
    Author: Dmitry Smirnov <onlyjob@member.fsf.org>
    Forwarded: 2013-01-10
    Bug-Upstream: http://scp.indiegames.us/mantis/view.php?id=2776
    Description: fixes FTBFS with format hardening
     Workaround for error: "format not a string literal and
     "no format arguments [-Werror=format-security]"
    
    --- a/code/network/chat_api.cpp
    +++ b/code/network/chat_api.cpp
    @@ -1107,9 +1107,9 @@
     	}
     	if(stricmp(szCmd,"432")==0)
     	{
     		//Channel Mode info
    -		snprintf(szResponse, SSIZE(szResponse), XSTR("Your nickname contains invalid characters", 640));
    +		snprintf(szResponse, SSIZE(szResponse), "%s", XSTR("Your nickname contains invalid characters", 640));
     		AddChatCommandToQueue(CC_DISCONNECTED,NULL,0);
     		return szResponse;
     	}
     	if(stricmp(szCmd,"433")==0)
    
    patch file icon hardening-format[trunk].patch (769 bytes) 2013-01-13 03:15 +

-Relationships
has duplicate 0002795closedniffiwan sprintf expects a format even if only a string is given 
+Relationships

-Notes

~0014626

Eli2 (developer)

I am not sure if changes like this will preserve the previous functionality.

- sprintf(str, XSTR("Unable to play movie %s.", 204), Cutscenes[which_cutscene].name);
+ sprintf(str, "%s", XSTR("Unable to play movie %s.", 204), Cutscenes[which_cutscene].name);

~0014628

niffiwan (developer)

Last edited: 2013-01-10 15:54

View 2 revisions

Also note, I could only get one hunk to apply vs trunk. In the 1st hunk of code/hud/hudtargetbox.cpp at least, all the sprintf calls have been changed to strcpy_s (r9255).

edit: actually, r9255 seems to have already fixed approx 80-90% of these. A few have been missed though.

~0014629

onlyjob (reporter)

Thank you very much for having a look.

2Eli2: yes it should be safe as far as I can tell.

2niffiwan: Indeed in trunk there is exactly one problem with format hardening in "code/network/chat_api.cpp" -- otherwise trunk builds with format hardening as I just verified.

~0014632

Echelon9 (developer)

Yes, I recall many of these had been fixed -- but that there was one remaining in the network chat api code.

Can you provide a revised patch and we'll take a look at committing it after review?

~0014646

onlyjob (reporter)

Attached, thanks. I hope it's OK.

~0014650

Echelon9 (developer)

Looks fine -- will wait for another coder to provide their view before committing to trunk. Thanks for the patch

~0014654

niffiwan (developer)

Looks good to me, should be fine to commit when SVN is available again.

~0014658

niffiwan (developer)

Fix committed to trunk@9508.
+Notes

+Related Changesets

-Issue History
Date Modified Username Field Change
2013-01-10 00:26 onlyjob New Issue
2013-01-10 00:26 onlyjob Status new => assigned
2013-01-10 00:26 onlyjob Assigned To => chief1983
2013-01-10 00:26 onlyjob File Added: hardening-format.patch
2013-01-10 15:27 Eli2 Note Added: 0014626
2013-01-10 15:49 niffiwan Note Added: 0014628
2013-01-10 15:54 niffiwan Note Edited: 0014628 View Revisions
2013-01-10 19:08 onlyjob Note Added: 0014629
2013-01-11 20:15 Echelon9 Note Added: 0014632
2013-01-11 20:16 Echelon9 Status assigned => code review
2013-01-13 03:15 onlyjob File Added: hardening-format[trunk].patch
2013-01-13 03:17 onlyjob Note Added: 0014646
2013-01-14 03:47 Echelon9 Note Added: 0014650
2013-01-18 21:57 niffiwan Note Added: 0014654
2013-01-22 04:21 niffiwan Changeset attached => fs2open trunk r9508
2013-01-22 04:21 niffiwan Note Added: 0014658
2013-01-22 04:21 niffiwan Status code review => resolved
2013-01-22 04:21 niffiwan Resolution open => fixed
2013-01-22 10:22 chief1983 Assigned To chief1983 => niffiwan
2013-01-22 10:22 chief1983 Status resolved => assigned
2013-01-22 10:22 chief1983 Status assigned => resolved
2013-02-16 05:43 niffiwan Relationship added has duplicate 0002795
+Issue History