tag + 493047 patch
thanks
Hi Alex,
the problem seems to that in some circumstances
OptionContainer::findoptionS and findoptionM seem to get NULL as char*
from the string returned by the conffile deque. To make the problem more
elusive, it actually disappears when one inserts enough debug output
statements. When testing with string::empty() before trying to process
the line, I even got it to segfault (on arm in qemu). Fixing the
iteration to be more sane (why the index should be signed in the
upstream author's opinion I do not want to know) makes everything work,
so that's what the attached patch does.
While the problem at hand is solved, I cannot help but comment on the
overall quality of the code: it is terrible. Just in the few places I
sprinkled debug statements in, I found as major problems
- the cast to (signed) of the array index, in addition to the strange
way of iterating through a deque,
- the handmade String (capital "S") class (derived from std::string) and
the very crude casting around and switching between string, String,
and char*,
- the mere idea of finding config options by loading the config file
into memory line by line and then iterating through the whole deal
for each valid config key is totally bogus.
Finding things like this when looking only at a tiny portion of the
config handling code is a really bad sign for things having anything to
do with the network and untrusted data.
Kind regards
T.
P.S.: This analysis was made possible by Aurelien Jarno's excellent
prebuilt qemu images, thanks!
--
Thomas Viehmann, http://thomas.viehmann.net/
diff -u dansguardian-2.9.9.7/debian/changelog dansguardian-2.9.9.7/debian/changelog
--- dansguardian-2.9.9.7/debian/changelog
+++ dansguardian-2.9.9.7/debian/changelog
@@ -1,3 +1,13 @@
+dansguardian (2.9.9.7-1.1) unstable; urgency=low
+
+ * Non-maintainer upload.
+ * OptionContainer.cpp: If you need to iterate through all lines in the
+ config file to find a field, at least don't abuse the configfile deque
+ by accessing it as if it was an array with signed index.
+ Works way better on arm, too, i.e. closes: #493047.
+
+ -- Thomas Viehmann <[EMAIL PROTECTED]> Sat, 11 Oct 2008 23:47:53 +0200
+
dansguardian (2.9.9.7-1) unstable; urgency=low
* New upstream version (Closes: #497260)
only in patch2:
unchanged:
--- dansguardian-2.9.9.7.orig/src/OptionContainer.cpp
+++ dansguardian-2.9.9.7/src/OptionContainer.cpp
@@ -662,8 +662,10 @@
String temp;
String temp2;
String o(option);
- for (int i = 0; i < (signed) conffile.size(); i++) {
- temp = conffile[i].c_str();
+ for (std::deque<std::string>::iterator i = conffile.begin(); i != conffile.end(); i++) {
+ if ((*i).empty())
+ continue;
+ temp = (*i).c_str();
temp2 = temp.before("=");
while (temp2.endsWith(" ")) { // get rid of tailing spaces before =
temp2.chop();
@@ -696,8 +698,10 @@
String temp2;
String o(option);
std::deque<String > results;
- for (int i = 0; i < (signed) conffile.size(); i++) {
- temp = conffile[i].c_str();
+ for (std::deque<std::string>::iterator i = conffile.begin(); i != conffile.end(); i++) {
+ if ((*i).empty())
+ continue;
+ temp = (*i).c_str();
temp2 = temp.before("=");
while (temp2.endsWith(" ")) { // get rid of tailing spaces before =
temp2.chop();
