Bug#778375: apt-transport-https: segfaults
On Mon, Feb 16, 2015 at 01:16:19AM +0100, Tomasz Buchert wrote:
The tricky HTTPS server returns this line: HTTP/1.1 302. Note that
there is no explanation for the status code 302 (it should be
Found). Anyway, this is fine, the code seems to be prepared for
that case: elements is set to 3 in server.cc:128.
apt is since 0.8.0~pre2 (23 Aug 2010). I think back then it was also
a sourceforge server triggering this. Note that this is a violation of
the HTTP1.1 spec (see rfc7230 section 3.1.2) which allows for an empty
reason-phrase, but the space before that is non-optional.
However, Owner is NULL (I don't know why, I don't know the code, but
it is) so Owner-Debug fails in server.cc:132.
The attached patch checks whether Owner is NULL before dereferencing
it. This fixes this problem for me, but somebody who knows what Owner
is should make sure that it makes sense. Feel free to adjust the
patch to your needs, it's in public domain.
rambling
That is a good catch! 'Owner' refers here to the ServerMethod owning the
ServerState (that was a very helpful explanation, wasn't it? ;) ).
It boils down to this: In Sep 2013 I wanted to fix some bugs in https by
using less curl and more of our own http code. For this I invented
a bunch of Server classes as parents for http and https – in handsight,
I really should have used another name, but well, anyway – expect that
both were completely different in their implementation.
Somehow I managed to pull it of anyway with the result that they share
most of their State parsing/tracking which is quite helpful. It also
means through that the actual Methods using the State are still very
different so getting a common interface for them was hard. Somewhere
down that line I took a shortcut giving the HttpsState a NULL for its
owner as it 'never' really needed it and can hence be fixed 'later'
correctly, right?
Fast forward one and a half years and the 'never' as well as the 'later'
is spoiled. Its a bit ironic that a debug message does this to me…
/rambling
The proposed patch works just fine as the other users for 'Owner'
aren't used by https and for http its always properly set (and nobody
dies if a debug message isn't shown even if requested) and at that point
in the release I guess everyone will be happy about a one-line fix.
(Michael is uploading it any minute now)
Attached is my fullblown 'proper' patch with a testcase I am going to
apply to our /experimental branch for comparison in the meantime.
Best regards
David Kalnischkies
diff --git a/methods/https.cc b/methods/https.cc
index 3a5981b..444bdef 100644
--- a/methods/https.cc
+++ b/methods/https.cc
@@ -109,7 +109,7 @@ HttpsMethod::progress_callback(void *clientp, double dltotal, double /*dlnow*/,
}
// HttpsServerState::HttpsServerState - Constructor /*{{{*/
-HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * /*Owner*/) : ServerState(Srv, NULL)
+HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * Owner) : ServerState(Srv, Owner)
{
TimeOut = _config-FindI(Acquire::https::Timeout,TimeOut);
Reset();
@@ -313,13 +313,11 @@ bool HttpsMethod::Fetch(FetchItem *Itm)
curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, timeout);
// set redirect options and default to 10 redirects
- bool const AllowRedirect = _config-FindB(Acquire::https::AllowRedirect,
- _config-FindB(Acquire::http::AllowRedirect,true));
curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, AllowRedirect);
curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 10);
// debug
- if(_config-FindB(Debug::Acquire::https, false))
+ if (Debug == true)
curl_easy_setopt(curl, CURLOPT_VERBOSE, true);
// error handling
@@ -356,7 +354,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm)
// go for it - if the file exists, append on it
File = new FileFd(Itm-DestFile, FileFd::WriteAny);
- Server = new HttpsServerState(Itm-Uri, this);
+ Server = CreateServerState(Itm-Uri);
// keep apt updated
Res.Filename = Itm-DestFile;
@@ -451,6 +449,25 @@ bool HttpsMethod::Fetch(FetchItem *Itm)
return true;
}
+ /*}}}*/
+// HttpsMethod::Configuration - Handle a configuration message /*{{{*/
+bool HttpsMethod::Configuration(string Message)
+{
+ if (ServerMethod::Configuration(Message) == false)
+ return false;
+
+ AllowRedirect = _config-FindB(Acquire::https::AllowRedirect,
+ _config-FindB(Acquire::http::AllowRedirect, true));
+ Debug = _config-FindB(Debug::Acquire::https,false);
+
+ return true;
+}
+ /*}}}*/
+ServerState * HttpsMethod::CreateServerState(URI uri) /*{{{*/
+{
+ return new HttpsServerState(uri, this);
+}
+ /*}}}*/
int main()
{
diff --git a/methods/https.h b/methods/https.h
index 411b714..f8d302d 100644
--- a/methods/https.h
+++ b/methods/https.h
@@ -52,7 +52,7 @@ class HttpsServerState : public ServerState
virtual ~HttpsServerState() {Close();};
};
-class HttpsMethod : public pkgAcqMethod
+class HttpsMethod : public ServerMethod
{
// minimum
Bug#778375: apt-transport-https: segfaults
On 15/02/15 23:55, Tomasz Buchert wrote:
[...]
(@Julian: sorry for not CCing you before)
Hi again,
I couldn't fall asleep, so there you go:
The tricky HTTPS server returns this line: HTTP/1.1 302. Note that
there is no explanation for the status code 302 (it should be
Found). Anyway, this is fine, the code seems to be prepared for
that case: elements is set to 3 in server.cc:128.
However, Owner is NULL (I don't know why, I don't know the code, but
it is) so Owner-Debug fails in server.cc:132.
The attached patch checks whether Owner is NULL before dereferencing
it. This fixes this problem for me, but somebody who knows what Owner
is should make sure that it makes sense. Feel free to adjust the
patch to your needs, it's in public domain.
Cheers,
Tomasz
From 3afccaefccc9045d5d1236f09d4cc90cc721c8ef Mon Sep 17 00:00:00 2001
From: Tomasz Buchert tomasz.buch...@inria.fr
Date: Mon, 16 Feb 2015 00:57:29 +0100
Subject: [PATCH] simple fix
---
methods/server.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/methods/server.cc b/methods/server.cc
index cb0341d..e321e02 100644
--- a/methods/server.cc
+++ b/methods/server.cc
@@ -129,7 +129,7 @@ bool ServerState::HeaderLine(string Line)
if (elements == 3)
{
Code[0] = '\0';
- if (Owner-Debug == true)
+ if (Owner != NULL Owner-Debug == true)
clog HTTP server doesn't give Reason-Phrase for Result std::endl;
}
else if (elements != 4)
--
2.1.4
Bug#778375: apt-transport-https: segfaults
Control: tag -1 unreproducible On Sun, Feb 15, 2015 at 09:19:29PM +0100, Tomasz Buchert wrote: On 14/02/15 10:44, Kurt Roeckx wrote: Package: apt-transport-https Version: 1.0.9.6 Severity: serious Hi, When I try to download something over https apt just segfaults: https[7809]: segfault at 69 ip 7f523b8cbb03 sp 7fff432589e0 error 4 in https[7f523b8c+12000] Kurt Hi Kurt, I cannot reproduce it: $ LC_ALL=C sudo apt-get install cowsay Reading package lists... Done Building dependency tree Reading state information... Done The following NEW packages will be installed: cowsay 0 upgraded, 1 newly installed, 0 to remove and 3 not upgraded. Need to get 20.0 kB of archives. After this operation, 92.2 kB of additional disk space will be used. Get:1 https://ftp-stud.hs-esslingen.de/debian/ testing/main cowsay all 3.03+dfsg1-10 [20.0 kB] Fetched 20.0 kB in 0s (65.9 kB/s) [... other standard things ...] Me neither (before and after a complete dist-upgrade today). Kurt: Are you up-to-date and could you recompile APT with debugging options and run that to get a backtrace? -- Julian Andres Klode - Debian Developer, Ubuntu Member See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/. Be friendly, do not top-post, and follow RFC 1855 Netiquette. - If you don't I might ignore you. -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#778375: apt-transport-https: segfaults
On 14/02/15 10:44, Kurt Roeckx wrote: Package: apt-transport-https Version: 1.0.9.6 Severity: serious Hi, When I try to download something over https apt just segfaults: https[7809]: segfault at 69 ip 7f523b8cbb03 sp 7fff432589e0 error 4 in https[7f523b8c+12000] Kurt Hi Kurt, I cannot reproduce it: $ LC_ALL=C sudo apt-get install cowsay Reading package lists... Done Building dependency tree Reading state information... Done The following NEW packages will be installed: cowsay 0 upgraded, 1 newly installed, 0 to remove and 3 not upgraded. Need to get 20.0 kB of archives. After this operation, 92.2 kB of additional disk space will be used. Get:1 https://ftp-stud.hs-esslingen.de/debian/ testing/main cowsay all 3.03+dfsg1-10 [20.0 kB] Fetched 20.0 kB in 0s (65.9 kB/s) [... other standard things ...] Cheers, Tomasz -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#778375: apt-transport-https: segfaults
On 15/02/15 23:16, Tomasz Buchert wrote:
[...]
Okay, I get a segfault too now:
[ 153.995036] https[2667]: segfault at 69 ip 7f41539d7b03 sp
7fffa171dbb0 error 4 in https[7f41539cc000+12000]
Tomasz
Hi again,
I've recompiled apt-transport-https with debugging symbols and
derandomized positions of code sections (via echo 0 | sudo tee
/proc/sys/kernel/randomize_va_space). I got this:
[ 510.536222] https[2990]: segfault at 69 ip fb03 sp
7fffdbf0 error 4 in https[4000+12000]
and then, via gdb:
(gdb) list *0xfb03
0xfb03 is in ServerState::HeaderLine(std::string)
(/tmp/apt-1.0.9.6/methods/server.cc:120).
115// Parse off any trailing spaces between the : and the next word.
116string::size_type Pos2 = Pos;
117while (Pos2 Line.length() isspace(Line[Pos2]) != 0)
118 Pos2++;
119
120string Tag = string(Line,0,Pos);
121string Val = string(Line,Pos2);
122
123if (stringcasecmp(Tag.c_str(),Tag.c_str()+4,HTTP) == 0)
124{
So there is an issue with parsing of HTTP headers or something like
that around server.cc:120. Unfortunately, I don't have much time to
dig more at the moment. Hope this helps.
Tomasz
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#778375: apt-transport-https: segfaults
On 15/02/15 22:22, Kurt Roeckx wrote: [...] Can you try adding this to your sources.list? deb https://dl.bintray.com/sbt/debian / And then apt-get install -d sbt Kurt Okay, I get a segfault too now: [ 153.995036] https[2667]: segfault at 69 ip 7f41539d7b03 sp 7fffa171dbb0 error 4 in https[7f41539cc000+12000] Tomasz -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#778375: apt-transport-https: segfaults
On Sun, Feb 15, 2015 at 09:19:29PM +0100, Tomasz Buchert wrote: On 14/02/15 10:44, Kurt Roeckx wrote: Package: apt-transport-https Version: 1.0.9.6 Severity: serious Hi, When I try to download something over https apt just segfaults: https[7809]: segfault at 69 ip 7f523b8cbb03 sp 7fff432589e0 error 4 in https[7f523b8c+12000] Kurt Hi Kurt, I cannot reproduce it: Can you try adding this to your sources.list? deb https://dl.bintray.com/sbt/debian / And then apt-get install -d sbt Kurt -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#778375: apt-transport-https: segfaults
Package: apt-transport-https Version: 1.0.9.6 Severity: serious Hi, When I try to download something over https apt just segfaults: https[7809]: segfault at 69 ip 7f523b8cbb03 sp 7fff432589e0 error 4 in https[7f523b8c+12000] Kurt -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
