Created attachment 14822 [details] [review]
0001-libFS-ansification.patch

Created attachment 14823 [details] [review]
0002-Remove-some-always-true-comparisons-due-to-structure.patch

Applied the first patch, but I'm not comfortable removing checks for
overflows, so I didn't apply the second one.

(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.

  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....