[devel] [APT PATCH] rpmSingle{Pkg, Src}Index::ArchiveURI(): avoid cases with undefined behavior

[devel] [APT PATCH] rpmSingle{Pkg, Src}Index::ArchiveURI(): avoid cases with undefined behavior

[Ivan Zakharyaschev]

Two cases of UB are avoided with such a rewrite:

* getcwd(2) returned NULL. Constructing a string from NULL is UB.
  (Such string was passed as an argument to flCombine().)
  Now, SafeGetCwd() (in fileutl.cc) returns "/" in such cases;
  if you consider SafeGetCwd()'s implementation not to be reasonable,
  rewrite it (just at a single place).

* File.length() < 2. Since File was a non-const string,
  File[File.length()] might be UB before C++11. Now, File is const, and
  it is guaranteed that File[File.length()] == 0.

This change was inspired by:

commit c138c48501381d26872a6b18aa1fb7af9f0300b2
Author: Aleksei Nikiforov <darktemplar@altlinux.org>
Date:   Mon Jun 10 18:10:30 2019 +0300

    RPM ArchiveURI: check file length before using it
---
 apt/apt-pkg/rpm/rpmindexfile.cc | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/apt/apt-pkg/rpm/rpmindexfile.cc b/apt/apt-pkg/rpm/rpmindexfile.cc
index 901ba77dd..ad6899a08 100644
--- a/apt/apt-pkg/rpm/rpmindexfile.cc
+++ b/apt/apt-pkg/rpm/rpmindexfile.cc
@@ -511,26 +511,20 @@ unsigned long rpmSrcDirIndex::Size() const
 
 // SinglePkgIndex::ArchiveURI - URI for the archive       	        /*{{{*/
 // ---------------------------------------------------------------------
-string rpmSinglePkgIndex::ArchiveURI(string File) const
+string rpmSinglePkgIndex::ArchiveURI(const string File) const
 {
-   char *cwd = getcwd(NULL,0);
-   if (File[0] == '.' && File[1] == '/')
-      File = string(File, 2);
-   string URI = "file://"+flCombine(cwd, File);
-   free(cwd);
-   return URI;
+   return "file://"+
+      flCombine(SafeGetCWD(),
+                (File[0] == '.' && File[1] == '/') ? string(File, 2) : File);
 }
 									/*}}}*/
 // SinglePkgIndex::ArchiveURI - URI for the archive       	        /*{{{*/
 // ---------------------------------------------------------------------
-string rpmSingleSrcIndex::ArchiveURI(string File) const
-{
-   char *cwd = getcwd(NULL,0);
-   if (File[0] == '.' && File[1] == '/')
-      File = string(File, 2);
-   string URI = "file://"+flCombine(cwd, File);
-   free(cwd);
-   return URI;
+string rpmSingleSrcIndex::ArchiveURI(const string File) const
+{
+   return "file://"+
+      flCombine(SafeGetCWD(),
+                (File[0] == '.' && File[1] == '/') ? string(File, 2) : File);
 }
 									/*}}}*/
 
-- 
2.21.0

Re: [devel] [APT PATCH] rpmSingle{Pkg, Src}Index::ArchiveURI(): avoid cases with undefined behavior

[Dmitry V. Levin]

On Sun, Feb 16, 2020 at 04:09:14AM +0300, Ivan Zakharyaschev wrote:
> Two cases of UB are avoided with such a rewrite:
> 
> * getcwd(2) returned NULL. Constructing a string from NULL is UB.
>   (Such string was passed as an argument to flCombine().)
>   Now, SafeGetCwd() (in fileutl.cc) returns "/" in such cases;
>   if you consider SafeGetCwd()'s implementation not to be reasonable,
>   rewrite it (just at a single place).

ack

> * File.length() < 2. Since File was a non-const string,
>   File[File.length()] might be UB before C++11. Now, File is const, and
>   it is guaranteed that File[File.length()] == 0.

We can safely assume C++11, but I don't think we have an UB here even
before C++11.  Anyway, this part of the change is better covered by
"Avoid copying objects" patch.


-- 
ldv

[devel] [nitpick] Re: [APT PATCH] rpmSingle{Pkg, Src}Index::ArchiveURI(): avoid cases with undefined behavior

[Michael Shigorin]

On Sun, Feb 16, 2020 at 02:17:00PM +0300, Dmitry V. Levin wrote:
> > * File.length() < 2. Since File was a non-const string,
> >   File[File.length()] might be UB before C++11. Now, File is const, and
> >   it is guaranteed that File[File.length()] == 0.
> We can safely assume C++11

...especially if it's spelled out explicitly with -std=.

-- 
 ---- WBR, Michael Shigorin / http://altlinux.org
  ------ http://opennet.ru / http://anna-news.info

Re: [devel] [APT PATCH] rpmSingle{Pkg, Src}Index::ArchiveURI(): avoid cases with undefined behavior

[Ivan Zakharyaschev]

On Sun, 16 Feb 2020, Dmitry V. Levin wrote:

> On Sun, Feb 16, 2020 at 04:09:14AM +0300, Ivan Zakharyaschev wrote:

> > * File.length() < 2. Since File was a non-const string,
> >   File[File.length()] might be UB before C++11. Now, File is const, and
> >   it is guaranteed that File[File.length()] == 0.
> 
> We can safely assume C++11, but I don't think we have an UB here even
> before C++11.

FWIW, after thinkng over the corresponding piece[1] which you cited (a 
third time):

[1]: https://en.cppreference.com/w/cpp/string/basic_string/operator_at

> reference       operator[]( size_type pos );(1)
> const_reference operator[]( size_type pos ) const;(2)
> 
> Returns a reference to the character at specified location pos. [...]
> 
> 1) If pos == size(), the behavior is undefined.
> 2) If pos == size(), a reference to the character with value CharT() 
> (the null character) is returned.
> 
> (until C++11)

I can see a reason to agree with you: whether the object is const might 
not be the thing that determines which variant of the method is used.

The question was whether variant (1) or (2) was used here in
(File[1] == '/') if File was not const.

(If (1) is used, it is stated that it would be UB until C++11; if (2), 
then not.)

I doubt I'm able remember all the details of how the choice is done 
between overloaded methods, but common sense gives a hint: only methods 
labelled "const" can be invoked on a const object whereas all methods 
(both labelled "const" and not) can be invoked on a non-const object.

Under this view, we see that the methods labelled "const" are equally well 
available for non-const objects. So, another factor must make the choice 
between (1) and (2).

And here the only remaining factor seems to be the expected result 
type/constness in the expression where it is used (although that might 
seem weird because of its "implicitness"): a const reference will do for 
(_ == '/'), so (2) is probably inferred under the hood as the variant to 
use here.

Of course, I might be wrong in my reasoning, and it's not quite important 
for this place in APT's code anymore anyway (as there was another reason 
to rewrite it), but I wished to share this curiosity regarding how the 
meaning of C++ code can be non-obvious to mere mortals (like me).

-- 
Best regards,
Ivan

Re: [devel] [APT PATCH] rpmSingle{Pkg, Src}Index::ArchiveURI(): avoid cases with undefined behavior

[Ivan Zakharyaschev]

On Mon, 17 Feb 2020, Ivan Zakharyaschev wrote:

> 
> On Sun, 16 Feb 2020, Dmitry V. Levin wrote:
> 
> > On Sun, Feb 16, 2020 at 04:09:14AM +0300, Ivan Zakharyaschev wrote:
> 
> > > * File.length() < 2. Since File was a non-const string,
> > >   File[File.length()] might be UB before C++11. Now, File is const, and
> > >   it is guaranteed that File[File.length()] == 0.
> > 
> > We can safely assume C++11, but I don't think we have an UB here even
> > before C++11.
> 
> FWIW, after thinkng over the corresponding piece[1] which you cited (a 
> third time):
> 
> [1]: https://en.cppreference.com/w/cpp/string/basic_string/operator_at
> 
> > reference       operator[]( size_type pos );(1)
> > const_reference operator[]( size_type pos ) const;(2)
> > 
> > Returns a reference to the character at specified location pos. [...]
> > 
> > 1) If pos == size(), the behavior is undefined.
> > 2) If pos == size(), a reference to the character with value CharT() 
> > (the null character) is returned.
> > 
> > (until C++11)
> 
> I can see a reason to agree with you: whether the object is const might 
> not be the thing that determines which variant of the method is used.
> 
> The question was whether variant (1) or (2) was used here in
> (File[1] == '/') if File was not const.
> 
> (If (1) is used, it is stated that it would be UB until C++11; if (2), 
> then not.)
> 
> I doubt I'm able remember all the details of how the choice is done 
> between overloaded methods, but common sense gives a hint: only methods 
> labelled "const" can be invoked on a const object whereas all methods 
> (both labelled "const" and not) can be invoked on a non-const object.
> 
> Under this view, we see that the methods labelled "const" are equally well 
> available for non-const objects. So, another factor must make the choice 
> between (1) and (2).
> 
> And here the only remaining factor seems to be the expected result 
> type/constness in the expression where it is used (although that might 
> seem weird because of its "implicitness"): a const reference will do for 
> (_ == '/'), so (2) is probably inferred under the hood as the variant to 
> use here.
> 
> Of course, I might be wrong in my reasoning, and it's not quite important 
> for this place in APT's code anymore anyway (as there was another reason 
> to rewrite it), but I wished to share this curiosity regarding how the 
> meaning of C++ code can be non-obvious to mere mortals (like me).

No, this reasoning must have been false, as pointed out by darktemplar@,  
by giving a link to the following explanations.

https://stackoverflow.com/a/164208/94687 :

The compiler does not take into account how you are using the return value 
in its determination; that's not part of the rules. It doesn't know if 
you're doing

    std::string name = b->Name();

or

    b->Name() = "me";

It has to choose the version that works in both cases.

https://stackoverflow.com/a/164130/94687 :

> There it noted that member functions may be overloaded merely by their 
> const attribute. In those cases, the compiler will use the member function 
> matching most closely the const-qualification of the object

-- 
Best regards,
Ivan