[PATCH 0/1] libopkg: Prevent blank lines in a package listing

[nhed+y...@starry.com]

From: Nevo Hed <nhed+y...@starry.com>

Not sure how this does not impact everyone. With the current version (I
suspect issue introduced in 0.4.2), I often see this on the SECOND
installation of a package.

remove_obsolesced_files: unlinking failed: No such file or directory

Note that due to lack of quotes it is hard to tell that the path is an
empty string.

Attempted to follow the submission guidelines and follow recommendations
from scripts/checkpatch.pl (such as removing 2 sets of braces that seemed
sensible to me) but the following warning was above my grasp - I fail to
see how my indentation is suspect (or inconsistent with the existing code,
and yeah I did specify `--ignore CODE_INDENT`).

```
$ scripts/checkpatch.pl 0001-libopkg-Prevent-blank-lines-in-a-package-listing.patch
WARNING: suspect code indent for conditional statements (8, 12)
#63: FILE: libopkg/pkg.c:1473:
+ if (key[1] == '\0' && key[0] == '/')
+ entry = xstrdup("/.");
```

Nevo Hed (1):
libopkg: Prevent blank lines in a package listing

libopkg/opkg_install.c | 4 ++--
libopkg/pkg.c | 7 ++++++-
2 files changed, 8 insertions(+), 3 deletions(-)

--
2.37.3

[nhed+y...@starry.com]

From: Nevo Hed <nhed+...@starry.com>

The data archive may contain an entry for the root dir.

70df4de added code to strip trailing slashes, and for the root dir a
single slash is removed, yielding an blank line in the middle of the
pkg.list.

An empty line in the listing would cause a subsequent install to emit
error logs (at end).

This change replaces a lone path of "/" with "/." (as observed in dpkg).

Sample log (with my added quotes):
---------------
Reinstalling iputils (2021.12.15-1) on root.
Installing iputils (2021.12.15) on root.
Removing obsolete file "".
* remove_obsolesced_files: unlinking "" failed: No such file or directory.
* opkg_install_pkg: Failed to determine obsolete files from previously installed iputils
Configuring iputils.
---------------

Signed-off-by: Nevo Hed <nhed+y...@starry.com>
---

libopkg/opkg_install.c | 4 ++--
libopkg/pkg.c | 7 ++++++-
2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/libopkg/opkg_install.c b/libopkg/opkg_install.c
index 354dc10..1a959cb 100644
--- a/libopkg/opkg_install.c
+++ b/libopkg/opkg_install.c
@@ -744,11 +744,11 @@ static int remove_obsolesced_files(pkg_t * pkg, pkg_t * old_pkg)
}

/* old file is obsolete */
- opkg_msg(NOTICE, "Removing obsolete file %s.\n", old->path);
+ opkg_msg(NOTICE, "Removing obsolete file \"%s\".\n", old->path);
if (!opkg_config->noaction) {
err = unlink(old->path);
if (err) {
- opkg_perror(ERROR, "unlinking %s failed", old->path);
+ opkg_perror(ERROR, "unlinking \"%s\" failed", old->path);
}
}
}
diff --git a/libopkg/pkg.c b/libopkg/pkg.c
index 6b1bd8f..4de3ff0 100644
--- a/libopkg/pkg.c
+++ b/libopkg/pkg.c
@@ -1468,7 +1468,12 @@ static void pkg_write_filelist_helper(const char *key, void *entry_,
size_t size;
int unmatched_offline_root = opkg_config->offline_root
&& !str_starts_with(key, opkg_config->offline_root);
- char *entry = xstrdup(key);
+ char *entry = NULL;
+

+ if (key[1] == '\0' && key[0] == '/')
+ entry = xstrdup("/.");

+ else
+ entry = xstrdup(key);

size = strlen(entry);
if (size > 0 && entry[size-1] == '/')
--
2.37.3

[Nevo Hed]

Additional note, not part of the patch itself, on the commit
introducing this (70df4de).

Introduction of a new variable named `entry` in the conditional scope
(as string), shadowing the `entry` at the function scope (as pkg_t )
makes reading the code difficult.

May I suggest renaming that inner `entry` to
1. `name`
2. `fname`
3. `fspath`
4. restore `key` and just rename the `key` function arg.

Willing to submit a follow-up patch for that but as is a style issue
would like feedback as to what's the best of the above(or other)
option for consistency with existing `opkg` code,

[Alex Stewart]

Hey Nevo,

Thanks for the patch. I think you've probably fixed a legitimate bug here.

A first pass of the PR looks good, but it'll be until tomorrow that I
can verify your fix.

On 10/16/22 00:21, nhed+y...@starry.com wrote:
> From: Nevo Hed <nhed+y...@starry.com>
>
> Not sure how this does not impact everyone. With the current version (I
> suspect issue introduced in 0.4.2), I often see this on the SECOND
> installation of a package.
>
> remove_obsolesced_files: unlinking failed: No such file or directory
>
> Note that due to lack of quotes it is hard to tell that the path is an
> empty string.
>
> Attempted to follow the submission guidelines and follow recommendations
> from scripts/checkpatch.pl (such as removing 2 sets of braces that seemed
> sensible to me) but the following warning was above my grasp - I fail to
> see how my indentation is suspect (or inconsistent with the existing code,
> and yeah I did specify `--ignore CODE_INDENT`).

Thanks for trying to follow the submission guidelines. Unfortunately,
they're pretty ancient at this point and I'm not surprised that they
don't make much sense. I'll see if I can fix them up.

>
> ```
> $ scripts/checkpatch.pl 0001-libopkg-Prevent-blank-lines-in-a-package-listing.patch
> WARNING: suspect code indent for conditional statements (8, 12)
> #63: FILE: libopkg/pkg.c:1473:
> + if (key[1] == '\0' && key[0] == '/')
> + entry = xstrdup("/.");
> ```
>
> Nevo Hed (1):
> libopkg: Prevent blank lines in a package listing
>
> libopkg/opkg_install.c | 4 ++--
> libopkg/pkg.c | 7 ++++++-
> 2 files changed, 8 insertions(+), 3 deletions(-)
>

Thanks!

--
Alex Stewart
Software Engineer - NI Real-Time OS
NI (National Instruments)

alex.s...@ni.com

[Alex Stewart]

On 10/16/22 00:21, nhed+y...@starry.com wrote:

> From: Nevo Hed <nhed+y...@starry.com>
>
> Not sure how this does not impact everyone. With the current version (I
> suspect issue introduced in 0.4.2), I often see this on the SECOND
> installation of a package.

Could you try to write up a test case for this behavior using the
regression testing framework in the `:tests/` directory? [1] You can use
the `:tests/regress/` cases as a reference.

It isn't totally obvious to me what conditions reproduce the behavior
you're seeing. And having a test case will help to keep this bug from
re-introducing.

[1] https://git.yoctoproject.org/opkg/tree/tests

> remove_obsolesced_files: unlinking failed: No such file or directory
>
> Note that due to lack of quotes it is hard to tell that the path is an
> empty string.

This issue might be larger than just the `pkg_write_filelist_helper()`
func that you fixed in this PR.

In commit 19d6e91f52ac9459a92e5ebe8395a732c9bd3fc3 [2], we added the
same "remove the trailing slash" logic to the
`pkg_get_installed_files()` function. And I think it might do the same
thing when handling the root path ("/"), and might commit an empty
string to the package object's file list.

Can you take a look at that function as well and see if we need to add a
check for that corner case?

cc. Shruthi Ravichandran - who was looking at this logic recently.

[2]
https://git.yoctoproject.org/opkg/commit/?id=19d6e91f52ac9459a92e5ebe8395a732c9bd3fc3

>
> Attempted to follow the submission guidelines and follow recommendations
> from scripts/checkpatch.pl (such as removing 2 sets of braces that seemed
> sensible to me) but the following warning was above my grasp - I fail to
> see how my indentation is suspect (or inconsistent with the existing code,
> and yeah I did specify `--ignore CODE_INDENT`).
>
> ```
> $ scripts/checkpatch.pl 0001-libopkg-Prevent-blank-lines-in-a-package-listing.patch
> WARNING: suspect code indent for conditional statements (8, 12)
> #63: FILE: libopkg/pkg.c:1473:
> + if (key[1] == '\0' && key[0] == '/')
> + entry = xstrdup("/.");
> ```
>
> Nevo Hed (1):
> libopkg: Prevent blank lines in a package listing
>
> libopkg/opkg_install.c | 4 ++--
> libopkg/pkg.c | 7 ++++++-
> 2 files changed, 8 insertions(+), 3 deletions(-)
>

--

[Nevo Hed]

On Tue, Oct 18, 2022 at 1:37 PM Alex Stewart <alex.s...@ni.com> wrote:
...

> Could you try to write up a test case for this behavior using the
> regression testing framework in the `:tests/` directory? [1] You can use
> the `:tests/regress/` cases as a reference.
>
> It isn't totally obvious to me what conditions reproduce the behavior
> you're seeing. And having a test case will help to keep this bug from
> re-introducing.

I might be able to get to it in a few days, but since python and I
don't get along well - so can I get a confirmation here?
Am I reading this correctly or are there are zero tests utilizing tar
mode? If so that's probably why it is not caught (our packages are
tar-duckens)

$ rg tar_not_ar
opk.py
66: def write(self, tar_not_ar=False, data_files=None, compression='gz'):
135: if tar_not_ar:
188: def write_opk(self, tar_not_ar=False, compression='gz'):
190: o.write(tar_not_ar=tar_not_ar, compression=compression)

[Alex Stewart]

On 10/20/22 11:09, Nevo Hed wrote:
> On Tue, Oct 18, 2022 at 1:37 PM Alex Stewart <alex.s...@ni.com> wrote:
> ...
>> Could you try to write up a test case for this behavior using the
>> regression testing framework in the `:tests/` directory? [1] You can use
>> the `:tests/regress/` cases as a reference.
>>
>> It isn't totally obvious to me what conditions reproduce the behavior
>> you're seeing. And having a test case will help to keep this bug from
>> re-introducing.
> I might be able to get to it in a few days, but since python and I
> don't get along well - so can I get a confirmation here?

I'm considering a rewrite to the opkg test framework. Is there anything
I could do to make it more approachable?

> Am I reading this correctly or are there are zero tests utilizing tar
> mode? If so that's probably why it is not caught (our packages are
> tar-duckens)
>
> $ rg tar_not_ar
> opk.py
> 66: def write(self, tar_not_ar=False, data_files=None, compression='gz'):
> 135: if tar_not_ar:
> 188: def write_opk(self, tar_not_ar=False, compression='gz'):
> 190: o.write(tar_not_ar=tar_not_ar, compression=compression)

You're right that none of the current tests exercise making the
*top-level* archive a TAR. eg. the IPK file is always an ar-archive.

But the `control` and `data` archives *within* the IPK are always
created as gzipped-tars [1] - regardless of if you enable `tar_not_ar`.
There's an argument that we should be exercising the other supported
compression types for those segments, but I don't think it affects this bug.

[1] https://git.yoctoproject.org/opkg/tree/tests/opk.py#n121