[PATCH] Fix for PR 21254 - Assertion in the comment parser
Dario Domizioli
dario.domizioli at gmail.com
Wed Oct 15 08:19:48 PDT 2014
Hello cfe-dev,
I have a patch to fix PR 21254 ( http://llvm.org/bugs/show_bug.cgi?id=21254
).
The details of the problem are quite complex and explained in the bugzilla
entry, but it all boils down to a bitfield being too narrow.
At the moment it is possible that a client of the Clang library may create
enough custom Doxygen-style commands that the comment parser runs out of ID
numbers it can use. Even Clang itself is using this registration mechanism
to record 'unknown' commands when it encounters them, and therefore a
maliciously constructed source code can crash Clang or cause some issues.
The problem is that this ID bitfield, whose original definition is in
CommentInfo class, is also replicated and embedded in other bitfields in
the Comment class, and the size of those bitfields is constrained to an
unsigned int (32 bits) unless we want to re-engineer the bitfields in the
Comment class to be larger.
So the patch does the following:
- It extends the ID bitfield from 8 to 20 bits (which is still in the
budget).
- It provides a DRY definition of the number of bits the field has, so that
we don't have "magic numbers" that have to agree in CommandInfo and Comment.
- It introduces an assertion that checks the number of IDs doesn't wrap
around.
- It adds the PR testcase as a Clang regression test that checks we can
have more than 256 commands.
Ideally we should have a test that checks for the maximum number of
commands, but with 20 bits we would need a million commands and the test
would be megabytes in size.
I can generate one if you think it's necessary, but I guess that checking
in a huge test file is not great.
Cheers,
Dario Domizioli
SN Systems - Sony Computer Entertainment Group
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141015/3ae4d493/attachment.html>
-------------- next part --------------
Index: include/clang/AST/Comment.h
===================================================================
--- include/clang/AST/Comment.h (revision 219791)
+++ include/clang/AST/Comment.h (working copy)
@@ -96,9 +96,10 @@
unsigned : NumInlineContentCommentBits;
unsigned RenderKind : 2;
- unsigned CommandID : 8;
+ unsigned CommandID : CommandInfo::NumCommandIDBits;
};
- enum { NumInlineCommandCommentBits = NumInlineContentCommentBits + 10 };
+ enum { NumInlineCommandCommentBits = NumInlineContentCommentBits + 2 +
+ CommandInfo::NumCommandIDBits };
class HTMLTagCommentBitfields {
friend class HTMLTagComment;
@@ -139,13 +140,14 @@
unsigned : NumCommentBits;
- unsigned CommandID : 8;
+ unsigned CommandID : CommandInfo::NumCommandIDBits;
/// Describes the syntax that was used in a documentation command.
/// Contains values from CommandMarkerKind enum.
unsigned CommandMarker : 1;
};
- enum { NumBlockCommandCommentBits = NumCommentBits + 9 };
+ enum { NumBlockCommandCommentBits = NumCommentBits +
+ CommandInfo::NumCommandIDBits + 1 };
class ParamCommandCommentBitfields {
friend class ParamCommandComment;
Index: include/clang/AST/CommentCommandTraits.h
===================================================================
--- include/clang/AST/CommentCommandTraits.h (revision 219791)
+++ include/clang/AST/CommentCommandTraits.h (working copy)
@@ -40,8 +40,12 @@
/// Name of the command that ends the verbatim block.
const char *EndCommandName;
- unsigned ID : 8;
+ /// DRY definition of the number of bits used for a command ID.
+ enum { NumCommandIDBits = 20 };
+ /// The ID of the command. Note that we can only have about a million IDs.
+ unsigned ID : NumCommandIDBits;
+
/// Number of word-like arguments for a given block command, except for
/// \\param and \\tparam commands -- these have special argument parsers.
unsigned NumArgs : 4;
Index: lib/AST/CommentCommandTraits.cpp
===================================================================
--- lib/AST/CommentCommandTraits.cpp (revision 219791)
+++ lib/AST/CommentCommandTraits.cpp (working copy)
@@ -89,6 +89,13 @@
// Value-initialize (=zero-initialize in this case) a new CommandInfo.
CommandInfo *Info = new (Allocator) CommandInfo();
Info->Name = Name;
+ // We only have a limited number of bits to encode command IDs in the
+ // CommandInfo structure, so the ID numbers can potentially wrap around and
+ // cause issues and assertions. In fact, this effect caused PR 21254.
+ // This assertion will catch the case where we have created too many commands
+ // to be encoded in the number of bits in the structure.
+ assert((NextID < (1 << CommandInfo::NumCommandIDBits))
+ && "Too many commands. We have limited bits for the command ID.");
Info->ID = NextID++;
RegisteredCommands.push_back(Info);
Index: test/Index/comment-lots-of-unknown-commands.c
===================================================================
--- test/Index/comment-lots-of-unknown-commands.c (revision 0)
+++ test/Index/comment-lots-of-unknown-commands.c (working copy)
@@ -0,0 +1,295 @@
+// RUN: c-index-test -test-load-source-reparse 1 local %s | FileCheck %s
+
+// See PR 21254. We had too few bits to encode command IDs so if you created
+// enough of them the ID codes would wrap around. This test creates 258
+// unknown commands. Ideally we should check for large numbers, but that would
+// require a test source file which is megabytes in size. This is the test case
+// from the PR.
+
+/**
+ at s
+ at tr
+ at y
+ at tt
+ at tg
+ at alu
+ at U
+ at I
+ at r
+ at t0
+ at t1
+ at ur
+ at S
+ at E
+ at pb
+ at f
+ at pe
+ at lue
+ at re
+ at oa
+ at l
+ at x
+ at R
+ at ute
+ at am
+ at ei
+ at oun
+ at ou
+ at nl
+ at ien
+ at fr
+ at en
+ at tet
+ at le
+ at L
+ at os
+ at A
+ at ro
+ at o
+ at ho
+ at ca
+ at Tie
+ at tl
+ at g
+ at hr
+ at et
+ at fro
+ at ast
+ at ae
+ at nN
+ at pc
+ at tae
+ at ws
+ at ia
+ at N
+ at lc
+ at psg
+ at ta
+ at t2
+ at D
+ at str
+ at ra
+ at t3
+ at t
+ at xt
+ at eN
+ at fe
+ at rU
+ at ar
+ at eD
+ at iE
+ at se
+ at st1
+ at rr
+ at ime
+ at ft
+ at lm
+ at wD
+ at wne
+ at h
+ at otn
+ at use
+ at roi
+ at ldc
+ at ln
+ at d
+ at ee
+ at ep
+ at us
+ at ut
+ at u
+ at n
+ at Nme
+ at min
+ at ma
+ at pct
+ at hd
+ at be
+ at It
+ at id
+ at cm
+ at ua
+ at fs
+ at Al
+ at axn
+ at rt
+ at to
+ at is
+ at fo
+ at i
+ at an
+ at de
+ at tel
+ at nd
+ at dic
+ at Lo
+ at il
+ at tle
+ at axt
+ at ba
+ at ust
+ at ac
+ at tpe
+ at tpl
+ at ctG
+ at ru
+ at m
+ at tG
+ at it
+ at rh
+ at G
+ at rpc
+ at el
+ at er
+ at w
+ at eo
+ at tx
+ at oo
+ at dD
+ at dD
+*/
+void f();
+
+// CHECK: CommentAST=[
+// CHECK: (CXComment_FullComment
+// CHECK: (CXComment_Paragraph
+// CHECK: (CXComment_InlineCommand CommandName=[s] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tr] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[y] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tt] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tg] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[alu] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[U] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[I] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[r] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[t0] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[t1] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ur] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[S] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[E] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[pb] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[f] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[pe] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[lue] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[re] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[oa] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[l] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[x] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[R] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ute] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[am] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ei] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[oun] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ou] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[nl] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ien] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[fr] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[en] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tet] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[le] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[L] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[os] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[A] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ro] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[o] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ho] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ca] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[Tie] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tl] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[g] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[hr] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[et] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[fro] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ast] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ae] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[nN] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[pc] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tae] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ws] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ia] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[N] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[lc] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[psg] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ta] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[t2] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[D] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[str] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ra] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[t3] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[t] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[xt] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[eN] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[fe] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[rU] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ar] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[eD] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[iE] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[se] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[st1] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[rr] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ime] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ft] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[lm] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[wD] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[wne] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[h] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[otn] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[use] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[roi] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ldc] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ln] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[d] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ee] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ep] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[us] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ut] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[u] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[Nme] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[min] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ma] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[pct] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[hd] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[be] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[It] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[id] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[cm] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ua] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[fs] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[Al] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[axn] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[rt] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[to] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[is] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[fo] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[i] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[an] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[de] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tel] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[nd] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[dic] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[Lo] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[il] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tle] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[axt] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ba] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ust] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ac] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tpe] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tpl] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ctG] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[ru] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[m] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tG] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[it] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[rh] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[G] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[rpc] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[el] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[er] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[w] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[eo] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[tx] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[oo] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[dD] RenderNormal HasTrailingNewline)
+// CHECK: (CXComment_InlineCommand CommandName=[dD] RenderNormal)))] Extent=[151:1 - 151:9]
More information about the cfe-commits
mailing list