From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=unavailable autolearn_force=no version=3.4.1 To: devel@lists.altlinux.org References: <20191211234857.GB17949@altlinux.org> <20191212095730.83787-1-darktemplar@altlinux.org> <20191212095730.83787-22-darktemplar@altlinux.org> <20191212220830.72763c8f12f951fceaec9d99@altlinux.org> From: Aleksei Nikiforov Message-ID: <592c4f67-d0d7-ae9a-1dfc-cae8f159092d@altlinux.org> Date: Fri, 13 Dec 2019 10:25:48 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20191212220830.72763c8f12f951fceaec9d99@altlinux.org> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Language: ru Content-Transfer-Encoding: 8bit Subject: Re: [devel] [PATCH for apt v2 21/21] Fix invalid check of Queue against zero X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Dec 2019 07:25:54 -0000 Archived-At: List-Archive: List-Post: 12.12.2019 22:08, Andrey Savchenko пишет: > On Thu, 12 Dec 2019 12:57:30 +0300 Aleksei Nikiforov wrote: >> Queue must not be zero in this function, otherwise it'd crash in this function >> anyway, since it's used like it's never zero later. >> Found via clang-static-analyzer: >> Logic error: Called C++ object pointer is null: >> Called C++ object pointer is null >> --- >> apt/apt-pkg/acquire-method.cc | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/apt/apt-pkg/acquire-method.cc b/apt/apt-pkg/acquire-method.cc >> index 9a3ef1d..3b5c580 100644 >> --- a/apt/apt-pkg/acquire-method.cc >> +++ b/apt/apt-pkg/acquire-method.cc >> @@ -555,9 +555,7 @@ void pkgAcqMethod::Warning(const char *Format,...) >> to keep the pipeline synchronized. */ >> void pkgAcqMethod::Redirect(const string &NewURI) >> { >> - string CurrentURI = ""; >> - if (Queue != 0) >> - CurrentURI = Queue->Uri; >> + string CurrentURI = Queue->Uri; > > If (Queue == NULL) this code will result in the NULL pointer > dereference. It does not follow from the code that Queue is never > NULL. > > So this may be a problem in the static analyzer. Even if it is not, > such safeguard elimination is not safe for future code > modifications. > > BTW GCC is good at DCE (dead code elimination), so if this check is > really useless, it will be omitted from the binary code. > If you read full code of this function, you'll see that even if Queue is NULL, it's still dereferenced later, and thus this check is excessive. No, it doesn't mean that it's never NULL. It just gets rid of excessive check. DCE is a good stuff, but it's much better to not keep it in source code if it's truly dead. >> ostringstream s; >> s << "103 Redirect\nURI: " << CurrentURI << "\nNew-URI: " << NewURI > > > Best regards, > Andrew Savchenko > > > _______________________________________________ > Devel mailing list > Devel@lists.altlinux.org > https://lists.altlinux.org/mailman/listinfo/devel >