From 626b7ccc8cec120f789ef89c2d5d28b8b6d67a04 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Tue, 11 Aug 2020 08:56:39 +0200 Subject: acquire: Do not hide _errror messages in Fail() If we have errors pending, always log them with our failure message to provide more context. --- apt-pkg/acquire-method.cc | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/apt-pkg/acquire-method.cc b/apt-pkg/acquire-method.cc index 9656caf14..486098c77 100644 --- a/apt-pkg/acquire-method.cc +++ b/apt-pkg/acquire-method.cc @@ -139,27 +139,30 @@ void pkgAcqMethod::SendMessage(std::string const &header, std::unordered_mapempty() == false) + + Fail("", Transient); +} + /*}}}*/ +// AcqMethod::Fail - A fetch has failed /*{{{*/ +void pkgAcqMethod::Fail(string Err, bool Transient) +{ + + if (not _error->empty()) { - Err.clear(); - while (_error->empty() == false) + while (not _error->empty()) { std::string msg; if (_error->PopMessage(msg)) { - if (Err.empty() == false) + if (not Err.empty()) Err.append("\n"); Err.append(msg); } } } - Fail(Err, Transient); -} - /*}}}*/ -// AcqMethod::Fail - A fetch has failed /*{{{*/ -void pkgAcqMethod::Fail(string Err,bool Transient) -{ + if (Err.empty()) + Err = "Undetermined Error"; + // Strip out junk from the error messages std::transform(Err.begin(), Err.end(), Err.begin(), [](char const c) { if (c == '\r' || c == '\n') -- cgit v1.2.3 From af7ab7c0002ef2cdfb1a4c0a468c5dbbda3d5dd0 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Tue, 11 Aug 2020 11:40:14 +0200 Subject: http: Restore successful exits from Die() We have successfully finished reading data if our buffer is empty, so we don't need to do any further checks. --- methods/http.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/methods/http.cc b/methods/http.cc index 77348d760..b9e82166c 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -709,10 +709,12 @@ ResultState HttpServerState::Die(RequestState &Req) // can't be set if (Req.File.Name() != "/dev/null") SetNonBlock(Req.File.Fd(),false); - if (In.WriteSpace()) { - _error->Error(_("Data left in buffer")); - return ResultState::TRANSIENT_ERROR; - } + + if (not In.WriteSpace() || In.IsLimit() == true || Persistent == false) + return ResultState::SUCCESSFUL; + + _error->Error(_("Data left in buffer")); + return ResultState::TRANSIENT_ERROR; } // See if this is because the server finished the data stream -- cgit v1.2.3 From 13ab2317451931f055855f1aeaec6c8b28b14ce2 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Tue, 11 Aug 2020 11:42:15 +0200 Subject: http: Do not use non-blocking local I/O This causes some more issues, really. --- methods/http.cc | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/methods/http.cc b/methods/http.cc index b9e82166c..9c06e030a 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -705,11 +705,6 @@ ResultState HttpServerState::Die(RequestState &Req) // Dump the buffer to the file if (Req.State == RequestState::Data) { - // on GNU/kFreeBSD, apt dies on /dev/null because non-blocking - // can't be set - if (Req.File.Name() != "/dev/null") - SetNonBlock(Req.File.Fd(),false); - if (not In.WriteSpace() || In.IsLimit() == true || Persistent == false) return ResultState::SUCCESSFUL; @@ -753,10 +748,6 @@ bool HttpServerState::Flush(FileFd * const File) { if (File != nullptr) { - // on GNU/kFreeBSD, apt dies on /dev/null because non-blocking - // can't be set - if (File->Name() != "/dev/null") - SetNonBlock(File->Fd(),false); if (In.WriteSpace() == false) return true; @@ -1031,7 +1022,6 @@ BaseHttpMethod::DealWithHeadersResult HttpMethod::DealWithHeaders(FetchResult &R if (Req.StartPos > 0) Res.ResumePoint = Req.StartPos; - SetNonBlock(Req.File.Fd(),true); return FILE_IS_OPEN; } /*}}}*/ -- cgit v1.2.3 From 73780d7f664a4ea1da55527d726b4c9c7753f1fb Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Tue, 11 Aug 2020 10:55:09 +0200 Subject: http: Fully flush local file both before/after server read We do not want to end up in a code path while reading content from the server where we have local data left to write, which can happen if a previous read included both headers and content. Restructure Flush() to accept a new argument to allow incomplete flushs (which do not match our limit), so that it can flush as far as possible, and modify Go() and use that before and after reading from the server. --- methods/basehttp.h | 2 +- methods/http.cc | 41 ++++++++++++++++++++++++----------------- methods/http.h | 2 +- 3 files changed, 26 insertions(+), 19 deletions(-) diff --git a/methods/basehttp.h b/methods/basehttp.h index 5fdff69e0..4a83f319c 100644 --- a/methods/basehttp.h +++ b/methods/basehttp.h @@ -107,7 +107,7 @@ struct ServerState virtual bool Close() = 0; virtual bool InitHashes(HashStringList const &ExpectedHashes) = 0; virtual ResultState Die(RequestState &Req) = 0; - virtual bool Flush(FileFd * const File) = 0; + virtual bool Flush(FileFd *const File, bool MustComplete = false) = 0; virtual ResultState Go(bool ToFile, RequestState &Req) = 0; virtual Hashes * GetHashes() = 0; diff --git a/methods/http.cc b/methods/http.cc index 9c06e030a..fb09bddd7 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -744,7 +744,7 @@ ResultState HttpServerState::Die(RequestState &Req) // --------------------------------------------------------------------- /* This takes the current input buffer from the Server FD and writes it into the file */ -bool HttpServerState::Flush(FileFd * const File) +bool HttpServerState::Flush(FileFd *const File, bool MustComplete) { if (File != nullptr) { @@ -759,7 +759,7 @@ bool HttpServerState::Flush(FileFd * const File) return true; } - if (In.IsLimit() == true || Persistent == false) + if (In.IsLimit() == true || Persistent == false || not MustComplete) return true; } return false; @@ -793,20 +793,23 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) if (In.ReadSpace() == true && ServerFd->Fd() != -1) FD_SET(ServerFd->Fd(), &rfds); - // Add the file - auto FileFD = MethodFd::FromFd(-1); - if (Req.File.IsOpen()) - FileFD = MethodFd::FromFd(Req.File.Fd()); - - if (In.WriteSpace() == true && ToFile == true && FileFD->Fd() != -1) - FD_SET(FileFD->Fd(), &wfds); + // Add the file. Note that we need to add the file to the select and + // then write before we read from the server so we do not have content + // left to write if the server closes the connection when we read from it. + // + // An alternative would be to just flush the file in those circumstances + // and then return. Because otherwise we might end up blocking indefinitely + // in the select() call if we were to continue but all that was left to do + // was write to the local file. + if (In.WriteSpace() == true && ToFile == true && Req.File.IsOpen()) + FD_SET(Req.File.Fd(), &wfds); // Add stdin if (Owner->ConfigFindB("DependOnSTDIN", true) == true) FD_SET(STDIN_FILENO,&rfds); // Figure out the max fd - int MaxFd = FileFD->Fd(); + int MaxFd = Req.File.Fd(); if (MaxFd < ServerFd->Fd()) MaxFd = ServerFd->Fd(); @@ -828,7 +831,15 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) _error->Error(_("Connection timed out")); return ResultState::TRANSIENT_ERROR; } - + + // Flush any data before talking to the server, in case the server + // closed the connection, we want to be done writing. + if (Req.File.IsOpen() && FD_ISSET(Req.File.Fd(), &wfds)) + { + if (not Flush(&Req.File, false)) + return ResultState::TRANSIENT_ERROR; + } + // Handle server IO if (ServerPending || (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &rfds))) { @@ -838,14 +849,10 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) } // Send data to the file - if (FileFD->Fd() != -1 && ((In.WriteSpace() == true && ToFile == true) || - FD_ISSET(FileFD->Fd(), &wfds))) + if (In.WriteSpace() == true && ToFile == true && Req.File.IsOpen()) { - if (In.Write(FileFD) == false) - { - _error->Errno("write", _("Error writing to output file")); + if (not Flush(&Req.File, false)) return ResultState::TRANSIENT_ERROR; - } } if (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &wfds)) diff --git a/methods/http.h b/methods/http.h index 5668f0b87..cae579afe 100644 --- a/methods/http.h +++ b/methods/http.h @@ -114,7 +114,7 @@ struct HttpServerState: public ServerState virtual bool InitHashes(HashStringList const &ExpectedHashes) APT_OVERRIDE; virtual Hashes * GetHashes() APT_OVERRIDE; virtual ResultState Die(RequestState &Req) APT_OVERRIDE; - virtual bool Flush(FileFd * const File) APT_OVERRIDE; + virtual bool Flush(FileFd *const File, bool MustComplete = true) APT_OVERRIDE; virtual ResultState Go(bool ToFile, RequestState &Req) APT_OVERRIDE; HttpServerState(URI Srv, HttpMethod *Owner); -- cgit v1.2.3 From 8b35e2a3dd7b863639a8909fa2361ed4fd217bc3 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Tue, 11 Aug 2020 13:09:14 +0200 Subject: Rewrite HttpServerState::Die() The old code was fairly confusing, and contradictory. Notably, the second `if` also only applied to the Data state, whereas we already terminated the Data state earlier. This was bad. The else fallback applied in three cases: (1) We reached our limit (2) We are Persistent (3) We are headers Now, it always failed as a transient error if it had nothing left in the buffer. BUT: Nothing left in the buffer is the correct thing to happen if we were fetching content. Checking all combinations for the flags, we can compare the results of Die() between 2.1.7 - the last "known-acceptable-ish" version and this version: 2.1.7 this Data !Persist !Space !Limit OK (A) OK Data !Persist !Space Limit OK (A) OK Data !Persist Space !Limit OK (C) OK Data !Persist Space Limit OK OK Data Persist !Space !Limit ERR ERR * Data Persist !Space Limit OK (B) OK Data Persist Space !Limit ERR ERR Data Persist Space Limit OK OK => Data connections are OK if they have not reached their limit, or are persistent (in which case they'll probably be chunked) Header !Persist !Space !Limit ERR ERR Header !Persist !Space Limit ERR ERR Header !Persist Space !Limit OK OK Header !Persist Space Limit OK OK Header Persist !Space !Limit ERR ERR Header Persist !Space Limit ERR ERR Header Persist Space !Limit OK OK Header Persist Space Limit OK OK => Common scheme here is that header connections are fine if they have read something into the input buffer (Space). The rest does not matter. (A) Non-persistent connections with !space always enter the else clause, hence success (B) no Space means we enter the if/else, we go with else because IsLimit(), and we succeed because we don't have space (C) Having space we do enter the while (WriteSpace()) loop, but we never reach IsLimit(), hence we fall through. Given that our connection is not persistent, we fall through to the else case, and there we win because we have data left to write. --- methods/http.cc | 46 ++++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/methods/http.cc b/methods/http.cc index fb09bddd7..0ef695166 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -702,40 +702,30 @@ ResultState HttpServerState::Die(RequestState &Req) Close(); - // Dump the buffer to the file - if (Req.State == RequestState::Data) + switch (Req.State) { - if (not In.WriteSpace() || In.IsLimit() == true || Persistent == false) - return ResultState::SUCCESSFUL; - - _error->Error(_("Data left in buffer")); - return ResultState::TRANSIENT_ERROR; + case RequestState::Data: + // We have read all data we could, or the connection is not persistent + if (In.IsLimit() == true || Persistent == false) + return ResultState::SUCCESSFUL; + break; + case RequestState::Header: + In.Limit(-1); + // We have read some headers, but we might also have read the content + // and an EOF and hence reached this point. This is fine. + if (In.WriteSpace()) + return ResultState::SUCCESSFUL; + break; } - // See if this is because the server finished the data stream - if (In.IsLimit() == false && Req.State != RequestState::Header && - Persistent == true) + // We have reached an actual error, tell the user about it. + if (LErrno == 0) { - if (LErrno == 0) - { - _error->Error(_("Error reading from server. Remote end closed connection")); - return ResultState::TRANSIENT_ERROR; - } - errno = LErrno; - _error->Errno("read", _("Error reading from server")); + _error->Error(_("Error reading from server. Remote end closed connection")); return ResultState::TRANSIENT_ERROR; } - else - { - In.Limit(-1); - - // Nothing left in the buffer - if (In.WriteSpace() == false) - return ResultState::TRANSIENT_ERROR; - - // We may have got multiple responses back in one packet.. - return ResultState::SUCCESSFUL; - } + errno = LErrno; + _error->Errno("read", _("Error reading from server")); return ResultState::TRANSIENT_ERROR; } -- cgit v1.2.3