Bug 347

Summary: fonttosfnt: fix for crash on x86_64
Product: xorg Reporter: Mike FABIAN <mfabian>
Component: Fonts/otherAssignee: Keith Packard <keithp>
Status: RESOLVED WONTFIX QA Contact:
Severity: normal    
Priority: high CC: eich
Version: unspecified   
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 213    
Attachments:
Description Flags
fonttosfnt-fix-crash-on-x86_64.diff none

Description Mike FABIAN 2004-03-18 07:51:02 UTC
I tried to use fonttosfnt from  the X.org tree from:

cvs -d :pserver:anoncvs@pdx.freedesktop.org:/cvs/xorg co -P -rXORG-RELEASE-1 xc

It crashes on x86_64.

The problem appears to be that vsnprintf is called more than once on
the same argument list. According to the man-page of the va_functions:

man va_start>    va_copy
man va_start>        An obvious implementation would have a va_list  a  pointer
man va_start>        to  the  stack  frame of the variadic function.  In such a
man va_start>        setup (by far the most common) there seems nothing against
man va_start>        an assignment
man va_start>                    va_list aq = ap;
man va_start>        Unfortunately,  there  are  also  systems  that make it an
man va_start>        array of pointers (of length 1), and there one needs
man va_start>                    va_list aq;
man va_start>                    *aq = *ap;
man va_start>        Finally, on systems where parameters are passed in  regis-
man va_start>        ters, it may be necessary for va_start to allocate memory,
man va_start>        store the parameters there,  and  also  an  indication  of
man va_start>        which  parameter  is next, so that va_arg can step through
man va_start>        the list. Now va_end can free the allocated memory  again.
man va_start>        To  accommodate  this situation, C99 adds a macro va_copy,
man va_start>        so that the above assignment can be replaced by
man va_start>                    va_list aq;
man va_start>                    va_copy(aq, ap);
man va_start>                    ...
man va_start>                    va_end(aq);
man va_start>        Each invocation of va_copy must be  matched  by  a  corre-
man va_start>        sponding  invocation of va_end in the same function.  Some
man va_start>        systems that do not supply va_copy have __va_copy instead,
man va_start>        since that was the name used in the draft proposal.

I.e. this won't work on some platforms. Indeed it crashes on x86_64
(AMD 64 bit system).

I tried to fix it with the help of va_copy. Unfortunately as written
in the man-page quoted above, va_copy is C99 therefore it might not be
available everywhere. Therefore my patch uses __va_copy if va_copy
is not defined. But

   - this is ugly
   - probably it still doesn't work everywhere.

How to fix this correctly?

My patch also makes vsprintf_reliable a static function, because it is
apparently only used in util.c.
Comment 1 Mike FABIAN 2004-03-18 07:51:44 UTC
Created attachment 154 [details] [review]
fonttosfnt-fix-crash-on-x86_64.diff

tentative patch
Comment 2 Bugzilla Maintainer 2004-03-18 11:35:51 UTC
I don't think this is a show stopper for the monolithic tree; if anything, it
demonstrates that this utility is not really ready for release and should be
removed from this distribution.  fonttosfnt isn't useful as it stands with this
release anyway as the necessary parts of the FreeType font loader aren't present.

I don't know why it was put into XFree86 4.4.
Comment 3 Egbert Eich 2004-03-24 08:45:54 UTC
Keith, should we apply this or wait since fontstosfnt doesn't seem to be usable
anyway. In this case we should leave it to Juliusz to fix it in whatever tree
he prefers.
Comment 4 Keith Packard 2004-03-24 09:10:41 UTC
I suggest we remove fonttosfnt from the normal build process and wait for it to
be ready for use.  Because it's already in cvs at /cvs/xapps/fonttosfnt, we
might also consider just deleting it from the monolithic tree.  There will never
be a need for this application in the normal build process for X; when it does
become useful, all of the existing BDF fonts will be replaced with .TTF files.
Comment 5 Egbert Eich 2004-03-24 10:35:37 UTC
OK, I will disable it for now so we can decide to delete it after the release.
I'll therefore close this with WONTFIX as we will not commit the applied patch.
I however know that this patch is known to Juliusz as I watched the conversation
between Mike and him.

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.