Badly formatted subpath for git SRC_URI deletes portions of hard drive.

Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[John Ernberg]

Hi

I was writing a bitbake recipe for a project hosted on a git-server. I 
applied subpath in order to limit the checkout to a specific directory.

When I ran the recipe it failed in do_fetch (or do_unpack) complaining 
about missing files. It had deleted the entire Yocto directory and 
selected directories from my home-dir like ssh-keys, my dir where I keep 
my code, and some configuration files for Thunderbird and my terminal.

Since I had no Yocto directory left I was unable to debug the recipe.

After restoring everything I re-created the recipe, this time I also 
saved a copy of the recipe at a safe location.
At the end of the path given to subpath I had added a '/' as it was not 
specified what the format of the path should be. I figured I would see 
an error if the path was wrong, and technically I did.
Essentially the SRC_URI looked like this:
SRC_URI = "git://git@<server+repo>;protocol=ssh;subpath=path/"

Running the recipe resulted in do_fetch (or do_unpack) failing again and 
complaining about missing files. This time the deletion was on a much 
greater scale, it had deleted so much of my home-dir that my user 
account was rendered entirely useless. The fastest way to recover this 
time was re-installing the machine.

I was keeping bitbake in my home-dir both of these times.

The poky version I am using is Daisy.

Best regards // John Ernberg

Re: Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[Paul Eggleton]

Hi John,

On Thursday 16 April 2015 11:52:48 John Ernberg wrote:
> I was writing a bitbake recipe for a project hosted on a git-server. I
> applied subpath in order to limit the checkout to a specific directory.
> 
> When I ran the recipe it failed in do_fetch (or do_unpack) complaining
> about missing files. It had deleted the entire Yocto directory and
> selected directories from my home-dir like ssh-keys, my dir where I keep
> my code, and some configuration files for Thunderbird and my terminal.
> 
> Since I had no Yocto directory left I was unable to debug the recipe.
> 
> After restoring everything I re-created the recipe, this time I also
> saved a copy of the recipe at a safe location.
> At the end of the path given to subpath I had added a '/' as it was not
> specified what the format of the path should be. I figured I would see
> an error if the path was wrong, and technically I did.
> Essentially the SRC_URI looked like this:
> SRC_URI = "git://git@<server+repo>;protocol=ssh;subpath=path/"
> 
> Running the recipe resulted in do_fetch (or do_unpack) failing again and
> complaining about missing files. This time the deletion was on a much
> greater scale, it had deleted so much of my home-dir that my user
> account was rendered entirely useless. The fastest way to recover this
> time was re-installing the machine.
> 
> I was keeping bitbake in my home-dir both of these times.
> 
> The poky version I am using is Daisy.

Very sorry about this :(

Looking at the code I can see at least two problems we need to fix relating to 
this issue. It seems that having '/' either at the start or the end of the 
value was unexpected and would trigger undesirable behaviour.

It may be worth noting in case it wasn't clear that when it is working 
correctly subpath= doesn't do a shallow clone, it still clones the full 
repository, it's just that you only get the specified subdirectory unpacked 
into the work directory after that.

- Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[Anders Darander]

* Paul Eggleton <paul.eggleton@linux.intel.com> [150416 14:57]:

> Hi John,

> On Thursday 16 April 2015 11:52:48 John Ernberg wrote:
> > After restoring everything I re-created the recipe, this time I also
> > saved a copy of the recipe at a safe location.
> > At the end of the path given to subpath I had added a '/' as it was not
> > specified what the format of the path should be. I figured I would see
> > an error if the path was wrong, and technically I did.
> > Essentially the SRC_URI looked like this:
> > SRC_URI = "git://git@<server+repo>;protocol=ssh;subpath=path/"

> > Running the recipe resulted in do_fetch (or do_unpack) failing again and
> > complaining about missing files. This time the deletion was on a much
> > greater scale, it had deleted so much of my home-dir that my user
> > account was rendered entirely useless. The fastest way to recover this
> > time was re-installing the machine.

It's most likely a call to bb.utils.prunedir(destdir), with destdir
being set to '/'...

> Very sorry about this :(

Yeah, too bad...

> Looking at the code I can see at least two problems we need to fix relating to 
> this issue. It seems that having '/' either at the start or the end of the 
> value was unexpected and would trigger undesirable behaviour.

The issue with having '/' at the end ought to be solved by adding
.rstrip('/') like:
----------------------------
diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py
index 44fc271..0fd9bee 100644
--- a/bitbake/lib/bb/fetch2/git.py
+++ b/bitbake/lib/bb/fetch2/git.py
@@ -246,7 +246,7 @@ class Git(FetchMethod):
         subdir = ud.parm.get("subpath", "")
         if subdir != "":
             readpathspec = ":%s" % (subdir)
-            def_destsuffix = "%s/" % os.path.basename(subdir)
+            def_destsuffix = "%s/" %
os.path.basename(subdir.rstrip('/'))
         else:
             readpathspec = ""
             def_destsuffix = "git/"
---------------------------

Probably, the first issue could be solved by replacing subdir with
subdir.strip('/') on the line above that.

Though I might be missing something...

I've only looked for a moment on the patch above, which at least seems
to create correct path's when inspecting the output.

Cheers,
Anders

-- 
Anders Darander
ChargeStorm AB / eStorm AB

Re: Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[Nicolas Dechesne]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Thu, Apr 16, 2015 at 3:11 PM, Anders Darander <anders@chargestorm.se>
wrote:

> > > Running the recipe resulted in do_fetch (or do_unpack) failing again
> and
> > > complaining about missing files. This time the deletion was on a much
> > > greater scale, it had deleted so much of my home-dir that my user
> > > account was rendered entirely useless. The fastest way to recover this
> > > time was re-installing the machine.
>
> It's most likely a call to bb.utils.prunedir(destdir), with destdir
> being set to '/'...



ouch... this is a bit scary ;-)

maybe we should assert the folder name is 'sane' when we do destructive
operations like that. e.g. check that we are in TMPDIR, or TOPDIR..

hopefully nobody does sudo bitbake ;-)

[-- Attachment #2: Type: text/html, Size: 1240 bytes --]

Re: Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]

On Thu, Apr 16, 2015 at 7:25 AM, Nicolas Dechesne <
nicolas.dechesne@linaro.org> wrote:

> On Thu, Apr 16, 2015 at 3:11 PM, Anders Darander <anders@chargestorm.se>
> wrote:
>
>> > > Running the recipe resulted in do_fetch (or do_unpack) failing again
>> and
>> > > complaining about missing files. This time the deletion was on a much
>> > > greater scale, it had deleted so much of my home-dir that my user
>> > > account was rendered entirely useless. The fastest way to recover this
>> > > time was re-installing the machine.
>>
>> It's most likely a call to bb.utils.prunedir(destdir), with destdir
>> being set to '/'...
>
>
>
> ouch... this is a bit scary ;-)
>
> maybe we should assert the folder name is 'sane' when we do destructive
> operations like that. e.g. check that we are in TMPDIR, or TOPDIR..
>
> hopefully nobody does sudo bitbake ;-)
>

This is scary indeed. We definitely need to be more careful about calls to
dirname/basename with paths with a trailing or leading / in general, and
especially for removals, and just need to do more path
sanitization/normalization probably..

>>> def dirsplit(p):
...     return os.path.dirname(p), os.path.basename(p)
...
>>> dirsplit('/usr/bin/foo')
('/usr/bin', 'foo')
>>> dirsplit('/usr/bin/')
('/usr/bin', '')
>>> dirsplit('/usr/bin')
('/usr', 'bin')
>>> dirsplit('/bin')
('/', 'bin')
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2964 bytes --]

Re: Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[Paul Eggleton]

On Thursday 16 April 2015 07:35:40 Christopher Larson wrote:
> On Thu, Apr 16, 2015 at 7:25 AM, Nicolas Dechesne <
> 
> nicolas.dechesne@linaro.org> wrote:
> > On Thu, Apr 16, 2015 at 3:11 PM, Anders Darander <anders@chargestorm.se>
> > 
> > wrote:
> >> > > Running the recipe resulted in do_fetch (or do_unpack) failing again
> >> 
> >> and
> >> 
> >> > > complaining about missing files. This time the deletion was on a much
> >> > > greater scale, it had deleted so much of my home-dir that my user
> >> > > account was rendered entirely useless. The fastest way to recover
> >> > > this
> >> > > time was re-installing the machine.
> >> 
> >> It's most likely a call to bb.utils.prunedir(destdir), with destdir
> >> being set to '/'...
> > 
> > ouch... this is a bit scary ;-)
> > 
> > maybe we should assert the folder name is 'sane' when we do destructive
> > operations like that. e.g. check that we are in TMPDIR, or TOPDIR..
> > 
> > hopefully nobody does sudo bitbake ;-)
> 
> This is scary indeed. We definitely need to be more careful about calls to
> dirname/basename with paths with a trailing or leading / in general, and
> especially for removals, and just need to do more path
> sanitization/normalization probably..

I have sent an additional patch to the bitbake list to try to protect against 
future instances of this issue, at least for bb.utils.prunedir() and 
bb.utils.remove():

http://lists.openembedded.org/pipermail/bitbake-devel/2015-April/005664.html

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[Anders Darander]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

On 17 Apr 2015 16:30, "Paul Eggleton" <paul.eggleton@linux.intel.com> wrote:
>
> I have sent an additional patch to the bitbake list to try to protect
against
> future instances of this issue, at least for bb.utils.prunedir() and
> bb.utils.remove():
>
>
http://lists.openembedded.org/pipermail/bitbake-devel/2015-April/005664.html

Good! That should give us some extra confidence for the future.

All the patches mentioned in this thread ought to be cherry-picked to all
currently supported Poky releases.

Thanks and cheers,
Anders

[-- Attachment #2: Type: text/html, Size: 824 bytes --]

Re: Badly formatted subpath for git SRC_URI deletes portions of hard drive.

[Paul Eggleton]

On Thursday 16 April 2015 15:11:44 Anders Darander wrote:
> * Paul Eggleton <paul.eggleton@linux.intel.com> [150416 14:57]:
> > Hi John,
> > 
> > On Thursday 16 April 2015 11:52:48 John Ernberg wrote:
> > > After restoring everything I re-created the recipe, this time I also
> > > saved a copy of the recipe at a safe location.
> > > At the end of the path given to subpath I had added a '/' as it was not
> > > specified what the format of the path should be. I figured I would see
> > > an error if the path was wrong, and technically I did.
> > > Essentially the SRC_URI looked like this:
> > > SRC_URI = "git://git@<server+repo>;protocol=ssh;subpath=path/"
> > > 
> > > Running the recipe resulted in do_fetch (or do_unpack) failing again and
> > > complaining about missing files. This time the deletion was on a much
> > > greater scale, it had deleted so much of my home-dir that my user
> > > account was rendered entirely useless. The fastest way to recover this
> > > time was re-installing the machine.
> 
> It's most likely a call to bb.utils.prunedir(destdir), with destdir
> being set to '/'...
> 
> > Very sorry about this :(
> 
> Yeah, too bad...
> 
> > Looking at the code I can see at least two problems we need to fix
> > relating to this issue. It seems that having '/' either at the start or
> > the end of the value was unexpected and would trigger undesirable
> > behaviour.
> 
> The issue with having '/' at the end ought to be solved by adding
> .rstrip('/') like:
> ----------------------------
> diff --git a/bitbake/lib/bb/fetch2/git.py b/bitbake/lib/bb/fetch2/git.py
> index 44fc271..0fd9bee 100644
> --- a/bitbake/lib/bb/fetch2/git.py
> +++ b/bitbake/lib/bb/fetch2/git.py
> @@ -246,7 +246,7 @@ class Git(FetchMethod):
>          subdir = ud.parm.get("subpath", "")
>          if subdir != "":
>              readpathspec = ":%s" % (subdir)
> -            def_destsuffix = "%s/" % os.path.basename(subdir)
> +            def_destsuffix = "%s/" %
> os.path.basename(subdir.rstrip('/'))
>          else:
>              readpathspec = ""
>              def_destsuffix = "git/"
> ---------------------------
> 
> Probably, the first issue could be solved by replacing subdir with
> subdir.strip('/') on the line above that.
> 
> Though I might be missing something...
> 
> I've only looked for a moment on the patch above, which at least seems
> to create correct path's when inspecting the output.

That looks like a reasonable solution to me, thanks.

FYI I've created the following bug to track this issue:

  https://bugzilla.yoctoproject.org/show_bug.cgi?id=7620

I'll send a fix ASAP probably based on the above.

Cheers,
Paul


-- 

Paul Eggleton
Intel Open Source Technology Centre