Bug 2245

Summary: xkb/ddxLoad.c cleanup
Product: xorg Reporter: Alexander Gottwald <ago>
Component: Server/GeneralAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: high CC: matthieu.herrb
Version: unspecified   
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 1865    
Bug Blocks:    
Attachments:
Description Flags
cleanup of ddxLoad.c
none
cleanup ddxList.c
none
Allocate xkbcomp cmdline buffer dynamicly
none
Allocate xkbcomp cmdline buffer dynamicly
none
Link libos.a after libxkb.a none

Description Alexander Gottwald 2005-01-09 07:22:37 UTC
ddxLoad is unreadable and unsafe. With the windows integration lot of security 
issues must be adressed and the code should be cleaned up to make it more
readable and maintainable

- #ifdef __UNIXOS2__ and WIN32 blocks should be reduced to a minimum
- more buffers should be checked for size before copying data to them
- the xkbcomp commandline should be allocated dynamicly (using Xprintf instead
of sprintf on static buffer)
Comment 1 Alexander Gottwald 2005-01-09 07:26:13 UTC
Created attachment 1645 [details] [review]
cleanup of ddxLoad.c

	* xc/programs/Xserver/xkb/ddxLoad.c:
	cleanup some #ifdef __UNIXOS2__ and WIN32 blocks.
	make OutputDirectory check the size of the buffer
	quote all file and pathnames in the xkbcomp commandline
	use PATH_MAX*4 for commandline buffer
Comment 2 Alexander Gottwald 2005-01-09 07:30:51 UTC
Comment on attachment 1645 [details] [review]
cleanup of ddxLoad.c

comitted to HEAD
/cvs/xorg/xc/ChangeLog,v  <--  ChangeLog
new revision: 1.649; previous revision: 1.648
/cvs/xorg/xc/programs/Xserver/xkb/ddxLoad.c,v  <--  ddxLoad.c
new revision: 1.8; previous revision: 1.7
Comment 3 Alexander Gottwald 2005-01-09 09:24:20 UTC
Created attachment 1647 [details] [review]
cleanup ddxList.c

	* xc/programs/Xserver/xkb/ddxList.c:
	* xc/programs/Xserver/xkb/ddxLoad.c:
	export Win32System and Win32TempDir
	remove #ifdef WIN32 block for building xkbcomp commandline
	create win32 tempfile in system tempdir
	use PATH_MAX*4 for commandline buffer
	unlink tempfile again
Comment 4 Alexander Gottwald 2005-01-09 09:36:38 UTC
Comment on attachment 1647 [details] [review]
cleanup ddxList.c

commited to HEAD
/cvs/xorg/xc/programs/Xserver/xkb/ddxList.c,v  <--  ddxList.c
new revision: 1.4; previous revision: 1.3
/cvs/xorg/xc/programs/Xserver/xkb/ddxLoad.c,v  <--  ddxLoad.c
new revision: 1.9; previous revision: 1.8
Comment 5 Alexander Gottwald 2005-01-09 09:57:51 UTC
Created attachment 1648 [details] [review]
Allocate xkbcomp cmdline buffer dynamicly

The buffer for the xkbcomp commandline is allocated dynamicly via Xprintf. This
removes the limits of the static buffer and the extra calculation of required
space which can easily get out of sync with the actual sprintf usage

Any objections?
Comment 6 Alexander Gottwald 2005-01-09 10:01:37 UTC
Created attachment 1649 [details] [review]
Allocate xkbcomp cmdline buffer dynamicly

removed another buffer length calculation which I missed in the previous patch
Comment 7 Matthieu Herrb 2005-01-13 14:26:30 UTC
xprintf.c lack a license. Please add an explicit one.
Comment 8 Alexander Gottwald 2005-01-14 04:18:28 UTC
added license statement to xprintf.c

/cvs/xorg/xc/ChangeLog,v  <--  ChangeLog
new revision: 1.681; previous revision: 1.680
/cvs/xorg/xc/programs/Xserver/os/xprintf.c,v  <--  xprintf.c
new revision: 1.2; previous revision: 1.1
Comment 9 Alexander Gottwald 2005-02-01 10:14:30 UTC
Comment on attachment 1649 [details] [review]
Allocate xkbcomp cmdline buffer dynamicly

/cvs/xorg/xc/ChangeLog,v  <--  ChangeLog
new revision: 1.737; previous revision: 1.736
/cvs/xorg/xc/programs/Xserver/xkb/ddxLoad.c,v  <-- 
programs/Xserver/xkb/ddxLoad.c
new revision: 1.10; previous revision: 1.9
/cvs/xorg/xc/programs/Xserver/xkb/ddxList.c,v  <-- 
programs/Xserver/xkb/ddxList.c
new revision: 1.5; previous revision: 1.4
Comment 10 Alexander Gottwald 2005-02-01 10:15:06 UTC
commited to HEAD. Marking as fixed...
Comment 11 Alexander Gottwald 2005-02-02 06:20:45 UTC
Created attachment 1817 [details] [review]
Link libos.a after libxkb.a

The use of Xprintf in libxkb.a requires libos.a to be linked after libxkb.a on
some systems. 

The patch adds a makro dependLib(x) which currently evaluates to x but can also
be configured to be silent for some platforms and adds dependLib($(OS)) to
XKBEXT to make sure libos.a is added after libxkb.a too.

Maybe the whole linking needs to be reworked. we currently have circular
dependencies between the libraries. dix need symbols from ddx, ddx needs
symbols from os, os needs symbols from dix and so forth.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.