1 <!DOCTYPE HTML PUBLIC
"-//W3C//DTD HTML 3.2//EN">
4 <TITLE> [IRCServices Coding] Analysis of the
5.1.2 bug
6 <LINK REL=
"Index" HREF=
"index.html" >
7 <LINK REL=
"made" HREF=
"mailto:ircservices-coding%40ircservices.za.net?Subject=%5BIRCServices%20Coding%5D%20Analysis%20of%20the%205.1.2%20bug&In-Reply-To=4726015c.15341%40msgid.achurch.org">
8 <META NAME=
"robots" CONTENT=
"index,nofollow">
9 <META http-equiv=
"Content-Type" content=
"text/html; charset=us-ascii">
10 <LINK REL=
"Previous" HREF=
"003312.html">
11 <LINK REL=
"Next" HREF=
"003314.html">
13 <BODY BGCOLOR=
"#ffffff">
14 <H1>[IRCServices Coding] Analysis of the
5.1.2 bug
</H1>
16 <A HREF=
"mailto:ircservices-coding%40ircservices.za.net?Subject=%5BIRCServices%20Coding%5D%20Analysis%20of%20the%205.1.2%20bug&In-Reply-To=4726015c.15341%40msgid.achurch.org"
17 TITLE=
"[IRCServices Coding] Analysis of the 5.1.2 bug">Craig at frostycoolslug.com
19 <I>Mon Oct
29 10:
39:
34 PDT
2007</I>
21 <LI>Previous message:
<A HREF=
"003312.html">[IRCServices Coding] Analysis of the
5.1.2 bug
23 <LI>Next message:
<A HREF=
"003314.html">[IRCServices Coding] samode
25 <LI> <B>Messages sorted by:
</B>
26 <a href=
"date.html#3313">[ date ]
</a>
27 <a href=
"thread.html#3313">[ thread ]
</a>
28 <a href=
"subject.html#3313">[ subject ]
</a>
29 <a href=
"author.html#3313">[ author ]
</a>
34 <PRE>No apology necessary my good friend, everyone screws up, even myself,
35 On one or two occasions i've written code which in theory was solid,
36 just to have it come back and bite me in the ass several times a few
37 weeks on, each time i fixed a part of it, another part came along and
38 took another bite (I'm missing half a buttock at this point) it's an
39 unfortunate part of the development cycle. Sure it's an embarrassment
40 and can induce a really bad day, but no one will think any less of
41 you, especially if they know the effort and general requirements which
42 go into making such a large package as services.
44 I've been on these lists now since
2001 (admittedly, I was a LOT
45 younger back then (
15ish?)), and i've found your general dedication to
46 the software to be inspirational, and it was tweaking IRCServices that
47 kickstarted my coding career. IRCServices has always been well
48 written, and always will be well written. I'm sure someone will pick
49 up the baton and continue the IRCServices package far into the future,
50 but i doubt they will have the same dedication and motivation to
51 improve this as you have had. I know from experience that coding
52 something for many, many years has some diminishing returns on the
53 mind of the developer (not that i'm saying you've lost your mind), but
54 dispite this, you've continued to fully support a package which you
55 have lost interest in.
57 You say you're a proud person when it comes to code, well, you damn
58 well should be. You've created a rock solid piece of software, you've
59 taken no crap over the years, and you've continued to evolve it to a
60 point where it's probably the best services package in existence.
62 Don't be depressed, don't be ashamed, don't be embarrassed, hold your
63 head up high, for the unfortunate events of the last week can't even
64 place a scratch on the quality you've provided over the years.
67 /**********************************************
68 * Craig
"FrostyCoolSlug
" McLure
69 * ChatSpike -
<A HREF=
"http://www.chatspike.net">http://www.chatspike.net
</A>
70 * InspIRCd -
<A HREF=
"http://www.inspircd.org">http://www.inspircd.org
</A>
71 **********************************************/
73 On
29/
10/
2007, Andrew Church
<<A HREF=
"http://lists.ircservices.za.net/mailman/listinfo/ircservices-coding">achurch at achurch.org
</A>> wrote:
74 ><i> Now that a bit of time has passed since the release of Services
75 </I>><i> 5.1.2, and (hopefully) everyone has upgraded, I thought I'd post a more
76 </I>><i> detailed explanation of exactly how the critical bug that prompted this
77 </I>><i> release came about. (As you are probably aware, various and sundry bugs
78 </I>><i> have pushed Services all the way to
5.1.6 since then, but in this text
79 </I>><i> I'll focus on the bug that was fixed by
5.1.2. Just as a reminder, all
80 </I>><i> 5.1.x releases through
5.1.5 have bugs allowing users to crash Services,
81 </I>><i> so you should immediately upgrade to
5.1.6 if you haven't already.)
86 </I>><i> The bug itself is fairly straightforward (as those who read the
87 </I>><i> diff file will have already realized): using the ChanServ AKICK VIEW
88 </I>><i> command on a channel with at least one autokick caused Services to
89 </I>><i> crash, without exception. By default, the AKICK command requires
90 </I>><i> channel access level
100 to use; this is the basis for the
"sufficiently
91 </I>><i> privileged users
" text in the announcement. In reality, unless channel
92 </I>><i> registration was disabled, any user could register a new channel (thus
93 </I>><i> gaining founder status) and use the AKICK command on that channel,
94 </I>><i> effectively allowing any user to crash Services. (Ironically, when
95 </I>><i> channel registration was disabled, a separate bug--fixed in
5.1.3--had
96 </I>><i> the potential to let any user cause a crash.)
98 </I>><i> A most embarrassing bug, indeed...
100 </I>><i> How It Came About
101 </I>><i> =================
103 </I>><i> One of the changes implemented in Services
5.1 was to unify (from
104 </I>><i> an interface standpoint, at least; internally, there's still far too
105 </I>><i> much code duplication) the listing functionality of all pseudoclient
106 </I>><i> commands. Among other things, this included removing the entry numbers
107 </I>><i> from the ChanServ ACCESS, XOP (SOP/AOP/HOP/VOP/NOP), and AKICK commands,
108 </I>><i> mostly to avoid the confusion that can result from multiple users
109 </I>><i> deleting entries in the middle of the list.
111 </I>><i> Conceptually, this is a simple change, only requiring the removal
112 </I>><i> of the corresponding parameter from the code that outputs entries to the
113 </I>><i> user. But theory and practice are different beasts, as they say; in the
114 </I>><i> case of AKICK VIEW, the data for each entry is formatted using a
115 </I>><i> language-specific string, requiring a change not only in the code itself
116 </I>><i> but also in each of the language files. I actually made this latter
117 </I>><i> change, presumably with the intent of going back and updating the code
118 </I>><i> once I had all the language files updates out of the way. However, it
119 </I>><i> seems that I forgot to make that final update, leaving the entry number
120 </I>><i> present in the the code that formatted autokick entries for the VIEW
121 </I>><i> subcommand. This resulted, eventually, in something like printf(
"%s
",
1)
122 </I>><i> --which of course crashed when the program tried to dereference the
123 </I>><i> entry number as a pointer.
125 </I>><i> Ordinarily, GCC can catch inconsistencies between format strings
126 </I>><i> and parameters like this; I always compile Services using GCC's -Werror
127 </I>><i> option, to ensure I catch as many potential problems as possible. With
128 </I>><i> language strings, however, the format string itself is located in a data
129 </I>><i> file separate from the source code. Since the data isn't accessible to
130 </I>><i> the compiler, these checks can't be made, and the error slipped by
131 </I>><i> unnoticed until this bug was reported.
133 </I>><i> Are Any Other Commands At Risk?
134 </I>><i> ===============================
136 </I>><i> With respect to this particular change, no. The removal of list
137 </I>><i> entry numbers affected only the ChanServ ACCESS LIST, XOP LIST, and
138 </I>><i> AKICK LIST/VIEW commands. Of these, XOP LIST and AKICK LIST use literal
139 </I>><i> format strings for their output, which were corrected as part of the
140 </I>><i> code change; ACCESS LIST uses the language system, but its code and
141 </I>><i> format strings were both updated properly, and it is likewise not
142 </I>><i> susceptible to this problem.
144 </I>><i> With respect to all commands in general, all I can say is
"I hope
145 </I>><i> not.
" I've done my best, within the constraints of time and an eleven-
146 </I>><i> year-old codebase, to program defensively; since
5.1.2, I've also gone
147 </I>><i> over all uses of language strings, and added some checks to my release
148 </I>><i> process (see below) to help avoid any more slipping in. But given that
149 </I>><i> I let these bugs by, who knows what else might be hiding?
151 </I>><i> Lessons To Be Learned
152 </I>><i> =====================
154 </I>><i> Lesson
1: Test suites
156 </I>><i> Ah, our good old friend, testing. I actually had started work on a
157 </I>><i> test suite for Services at one point, which was the main motivation for
158 </I>><i> the OperServ debug commands. Regrettably, between a lack of time in my
159 </I>><i> earlier days of Services development (being not nearly as fluent in
160 </I>><i> progamming then as I am now, writing a test suite to interact with a
161 </I>><i> server was a formidable task, and I chose to use my time working on
162 </I>><i> Services itself instead) and a lack of interest later, I never took the
163 </I>><i> test suite beyond simple nickname operations.
165 </I>><i> Of course, even a basic test that just ran through all the commands
166 </I>><i> would have caught this bug, and to that I can only say: Mea culpa.
167 </I>><i> Lack of interest or otherwise, I failed to follow best practices, and
168 </I>><i> that failure came back to bite me.
170 </I>><i> Lesson
2: Format strings are dangerous
172 </I>><i> Services makes use of the standard printf()-style format strings in
173 </I>><i> its language files, partly out of convenience and partly because I
174 </I>><i> didn't know any better when I designed the language system. While the
175 </I>><i> flexibility of format strings is undeniably useful, their implementation
176 </I>><i> leaves much to be desired--especially in a language like C that does not
177 </I>><i> pass type information around. In fact, inconsistencies between format
178 </I>><i> strings in different language files has resulted in similar crashes in
179 </I>><i> the past (including the less-dangerous one fixed in version
5.1.3). Of
180 </I>><i> course, most of the danger in using format strings comes from the fact
181 </I>><i> that strings are second-class citizens in the C data type world: to
182 </I>><i> process a string, you access successive bytes through a pointer--and if
183 </I>><i> that particular value happens to be something that's not a pointer,
184 </I>><i> BOOM. So this lesson could just as easily be
"C strings are dangerous
".
186 </I>><i> So what can be done about it? Well, one of the obvious solutions--
187 </I>><i> and probably the best idea in the long run--is to rewrite the
188 </I>><i> pseudoclients in a higher-level language, such as Perl; Perl's dynamic
189 </I>><i> typing ensures that this sort of problem can't occur. (Then again,
190 </I>><i> dynamically typed languages have their own problems, so this isn't a
191 </I>><i> perfect solution.) I'd actually had thoughts about implementing a Perl
192 </I>><i> base for pseudoclients in a future version of Services, but I don't have
193 </I>><i> nearly enough time or interest to do so at this point.
195 </I>><i> Another option would be to change the language file system to use a
196 </I>><i> custom formatting system. Parameters could be named, for example, and
197 </I>><i> an array of structures including parameter names and data types along
198 </I>><i> with the parameters themselves could be passed to routines like
199 </I>><i> notice_lang(). Of course, this has the downside of turning what's
200 </I>><i> currently a simple list of parameters into a more complex variable
201 </I>><i> declaration; and since the data types would have to be specified
202 </I>><i> manually, there's still the danger of mismatched types, leading to
203 </I>><i> potential crashes. Nonetheless, this might be a more feasible approach
204 </I>><i> for those who are interested in improving Services' robustness but don't
205 </I>><i> want to go as far as rewriting half of it in Perl.
207 </I>><i> I've implemented two crude solutions in Services
5.1.3. One is the
208 </I>><i> use of a script, modified from one provided by German translator Jacek
209 </I>><i> Margos, to check all language files for format string inconsistencies at
210 </I>><i> release time. The other is a simple compilation check: at release time,
211 </I>><i> I run a test compilation where notice_lang() and notice_help() calls
212 </I>><i> using fixed message IDs are replaced with notice() calls that use the
213 </I>><i> corresponding strings from the English language file (lang/en_us.l).
214 </I>><i> The resulting executable is of course useless from a multilingual point
215 </I>><i> of view, but the compiler will complain about any cases where format
216 </I>><i> strings and parameters don't match. While this won't catch cases where
217 </I>><i> the format string is selected by a variable, it would have found the
218 </I>><i> particular bug fixed in version
5.1.2.
220 </I>><i> Final Thoughts
221 </I>><i> ==============
223 </I>><i> Needless to say, this bug in particular, as well as the fact that I
224 </I>><i> had to make five releases of a supposedly
"stable
" version of Services
225 </I>><i> in the space of ten days to correct other bugs of varying seriousness,
226 </I>><i> is quite embarrassing. I'm of course acutely aware of the trouble it's
227 </I>><i> caused everyone using Services, of the repeated interruptions in normal
228 </I>><i> network operations that will have resulted from upgrades (if not
229 </I>><i> crashes)--and let none say that it's
"just IRC
"! But at the same time,
230 </I>><i> I take pride in my work as a software developer, and seeing myself make
231 </I>><i> such trivial mistakes as these is, well, depressing to say the least.
232 </I>><i> I could make excuses that the code base is aging (which is true), or
233 </I>><i> that I've lost too much interest in the program to do a proper job
234 </I>><i> (which is also true, to be honest--that's the main reason I declared
5.1
235 </I>><i> to be the final version of Services). I could even claim that I've got
236 </I>><i> another project twice the sice of Services that's considerably more
237 </I>><i> stable (which is true as well, thanks in no small part to the experience
238 </I>><i> I've gained from Services itself). But since none of that changes the
239 </I>><i> facts, all I can say in the end is that I screwed up, and I'm sorry.
241 </I>><i> --Andrew Church
242 </I>><i> <A HREF=
"http://lists.ircservices.za.net/mailman/listinfo/ircservices-coding">achurch at achurch.org
</A>
243 </I>><i> <A HREF=
"http://achurch.org/">http://achurch.org/
</A>
244 </I>><i> ------------------------------------------------------------------
245 </I>><i> To unsubscribe or change your subscription options, visit:
246 </I>><i> <A HREF=
"http://lists.ircservices.za.net/mailman/listinfo/ircservices-coding">http://lists.ircservices.za.net/mailman/listinfo/ircservices-coding
</A>
255 <LI>Previous message:
<A HREF=
"003312.html">[IRCServices Coding] Analysis of the
5.1.2 bug
257 <LI>Next message:
<A HREF=
"003314.html">[IRCServices Coding] samode
259 <LI> <B>Messages sorted by:
</B>
260 <a href=
"date.html#3313">[ date ]
</a>
261 <a href=
"thread.html#3313">[ thread ]
</a>
262 <a href=
"subject.html#3313">[ subject ]
</a>
263 <a href=
"author.html#3313">[ author ]
</a>
projects / irc.git / blob