Bug 14813 - xorg/lib/libFS - ansification and some fixes
Summary: xorg/lib/libFS - ansification and some fixes
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Lib/Xfont (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2008-03-04 13:19 UTC by Paulo César Pereira de Andrade
Modified: 2009-02-02 10:08 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
0001-libFS-ansification.patch (27.32 KB, patch)
2008-03-04 13:19 UTC, Paulo César Pereira de Andrade
no flags Details | Splinter Review
0002-Remove-some-always-true-comparisons-due-to-structure.patch (1.81 KB, patch)
2008-03-04 13:20 UTC, Paulo César Pereira de Andrade
no flags Details | Splinter Review

Description Paulo César Pereira de Andrade 2008-03-04 13:19:16 UTC
Ansification patch should not cause any problems, as there is no
promotable types, so no risk of binary incompatibility.

  The second patch just removes some compile warnings due to
"impossible" condition checks.
Comment 1 Paulo César Pereira de Andrade 2008-03-04 13:19:58 UTC
Created attachment 14822 [details] [review]
0001-libFS-ansification.patch
Comment 2 Paulo César Pereira de Andrade 2008-03-04 13:20:17 UTC
Created attachment 14823 [details] [review]
0002-Remove-some-always-true-comparisons-due-to-structure.patch
Comment 3 Alan Coopersmith 2008-04-24 19:31:43 UTC
Applied the first patch, but I'm not comfortable removing checks for
overflows, so I didn't apply the second one.
Comment 4 Paulo César Pereira de Andrade 2008-04-24 21:01:45 UTC
(In reply to comment #3)
> Applied the first patch, but I'm not comfortable removing checks for
> overflows, so I didn't apply the second one.

  Thanks. Actually patch2 should be reviewed to add a better/proper
check. Only the test in src/FSGetCats.c:FSGetCatalogues(), testing
field num_catalogs of a fsListCataloguesReply structure is actually
testing a 32 bit field, that I think is 32 bits just for padding...

  The other tests are impossible conditions as the tested field is a
CARD8, but probably the length field value should be calculated,
and checked if match. It should be a function like
rep.length == (rep.num_fields * some_size) + sizeof(some_struct) + (possible_padding)
  But looking at the code, it seens that an arbitrary amount of
data can come after the "useful" data (probably to allow "extensions"
transparently working), so it probably should just check if the
"num_fields" has a sane value, and the request lenght isn't too small,
or it would end up reading past the end of the memory holding the
request.

  Someone with more knowledge could review it :-) As I believe the
tests are to prevent attacks, and the test on a 32 bit field probably
should check for a sane value, like USHORT_MAX on rep.length, or
probably less, instead of SIZE_MAX / sizeof(char*), where size max
is either UINT_MAX or ULONG_MAX.
Comment 5 Paulo César Pereira de Andrade 2009-02-02 10:08:40 UTC
  Patch 1 already in git master.

  Newer gcc is not generating warnings about the "always true" check
(probably because gcc is internally, silently "automatically" removing
the impossible condition):

  if (<test1> &&
      <unsigned char variable> >= UINTMAX / sizeof(unsigned int) &&
      <test3>)

so, should probably not apply patch 2, to avoid confusion about the
reason of removing that check....


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.