Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests fail if UID is too large - Easily Reproducible #2109

Open
Volker-Weissmann opened this issue Apr 4, 2024 · 3 comments
Open

Unit tests fail if UID is too large - Easily Reproducible #2109

Volker-Weissmann opened this issue Apr 4, 2024 · 3 comments

Comments

@Volker-Weissmann
Copy link

Long story short:
If you run these commands

$ useradd -m highid --uid 1000000
useradd warning: highid's uid 1000000 outside of the UID_MIN 1000 and UID_MAX 60000 range.
$ su highid
$ cd
$ git clone https://github.com/libarchive/libarchive.git
$ cd libarchive
$ /bin/sh build/autogen.sh
$ ./configure
$ make
$ make check

Some tests fail:

PASS: libarchive_test
FAIL: bsdtar_test
PASS: bsdcpio_test
PASS: bsdcat_test
PASS: bsdunzip_test
============================================================================
Testsuite summary for libarchive 3.7.3
============================================================================
# TOTAL: 5
# PASS:  4
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
Please report to libarchive-discuss@googlegroups.com
============================================================================
Totals:
  Tests run:               72
  Tests failed:             8
  Assertions checked:   83264
  Assertions failed:      932
  Skips reported:           7

Failing tests:
  1: test_basic (14 failures)
  2: test_copy (886 failures)
  29: test_option_b (8 failures)
  34: test_option_gid_gname (8 failures)
  35: test_option_group (8 failures)
  53: test_option_owner (3 failures)
  56: test_option_r (2 failures)
  59: test_option_uid_uname (3 failures)
cat /tmp/bsdtar_test.2024-04-05T00.06.59-000/test_basic.log 
test_basic

tar/test/test_basic.c:104: File should be empty: pack.err
    File size: 108
    Contents:
0000 3a 20 4e 75 6d 65 72 69 63 20 75 73 65 72 20 49 : Numeric user I
0010 44 20 74 6f 6f 20 6c 61 72 67 65 3a 20 4e 75 6d D too large: Num
0020 65 72 69 63 20 75 73 65 72 20 49 44 20 74 6f 6f eric user ID too
0030 20 6c 61 72 67 65 3a 20 4e 75 6d 65 72 69 63 20  large: Numeric 
0040 75 73 65 72 20 49 44 20 74 6f 6f 20 6c 61 72 67 user ID too larg
0050 65 3a 20 4e 75 6d 65 72 69 63 20 75 73 65 72 20 e: Numeric user 
0060 49 44 20 74 6f 6f 20 6c 61 72 67 65             ID too large
tar/test/test_basic.c:61: File should exist: file
   Description: copy_ustar
tar/test/test_basic.c:63: File file has size 115, expected 10
   Description: copy_ustar
tar/test/test_basic.c:65: File should exist: file
   Description: copy_ustar
tar/test/test_basic.c:67: File file has 99151242690446 links, expected 2
   Description: copy_ustar
tar/test/test_basic.c:71: File should exist: linkfile
   Description: copy_ustar
tar/test/test_basic.c:73: File linkfile has size 115, expected 10
   Description: copy_ustar
tar/test/test_basic.c:74: File should exist: linkfile
tar/test/test_basic.c:75: File linkfile has 99151242682180 links, expected 2
tar/test/test_basic.c:76: File should exist: file
tar/test/test_basic.c:76: Files file and linkfile are not hardlinked
tar/test/test_basic.c:80: Symlink should exist: symlink
tar/test/test_basic.c:80: File symlink is not a symlink to file
tar/test/test_basic.c:84: Dir should exist: dir
   Description: copy_ustar
tar/test/test_basic.c:76: Summary: Failed 2 times
tar/test/test_basic.c:80: Summary: Failed 2 times

It would be better if those tests would not fail, but would be skipped.

If you want me to, I am happy to work on a PR that fixes the issue.

@kientzle
Copy link
Contributor

kientzle commented Apr 5, 2024

This is testing the basic "ustar" format, which only supports UIDs with up to 6 octal digits (which means that it cannot store UIDs larger than 256k). If you run the test suite verbosely bsdtar_test -v (you'll need other options as well), then you can see all the failures and why they're happening. Are all of them because of the same "Numeric user ID too large"?

I'd be happy to have someone work on fixing this, but I don't like the idea of skipping all of these tests if there's a reasonable alternative. (Skipping tests can end up accidentally hiding real problems.)

  • It might make sense to change some of these tests to use other formats. The point of the bsdtar tests is to verify that the CLI works correctly; the libarchive tests actually verify the detailed handling of each format. So using other formats could make sense for many of these tests.
  • The very oldest tar implementations would only use 6 bytes out of the 8 bytes available. Libarchive strictly follows this when "ustar" format has been requested but will use up to 8 bytes when writing more modern tar variants. It might make sense to relax this restriction some and allow 7 bytes even for strict ustar. I haven't looked carefully at how GNU tar, star, or other tar implementations handle the case of a large UID with strict ustar format.
  • The "ustar" format could in theory remap UIDs, but that would cause complications for some use cases so I don't think we should do that.

@kientzle
Copy link
Contributor

kientzle commented Apr 5, 2024

To be precise, my idea of using up to 7 bytes would mean changing this bit of code in archive_write_set_format_ustar.c

        if (format_number(archive_entry_uid(entry),
            h + USTAR_uid_offset, USTAR_uid_size, USTAR_uid_max_size, strict)) {
                archive_set_error(&a->archive, ERANGE,
                    "Numeric user ID too large");
                ret = ARCHIVE_FAILED;
        }

to something like the following, which would try formatting the UID with 7 digits if 6 was not enough:

       /* Try formatting the UID first with 6 digits (the historic maximum)
        * and if that fails, try 7.  The second attempt isn't helpful if we're not
        * strict here, since non-strict formatting expands as necessary. */
        if (format_number(archive_entry_uid(entry),
            h + USTAR_uid_offset, USTAR_uid_size, USTAR_uid_max_size, strict)
         && (!strict || format_number(archive_entry_uid(entry),
            h + USTAR_uid_offset, USTAR_uid_size + 1, USTAR_uid_max_size, strict))) {
                archive_set_error(&a->archive, ERANGE,
                    "Numeric user ID too large");
                ret = ARCHIVE_FAILED;
        }

(It might be clearer to change "format_number" to accept a "relaxed size" that will be used even in "strict" mode. That would make it easier to extend this idea to the mode, GID, rdevmajor, and rdevminor fields as well.)

Again, before doing this, I'd like to see if other tar implementations have tried this as well.

@Volker-Weissmann
Copy link
Author

Are all of them because of the same "Numeric user ID too large"?

I have not checked all error messages, but if I use a user with a small UID and small GID, all tests pass.

I'd be happy to have someone work on fixing this, but I don't like the idea of skipping all of these tests if there's a reasonable alternative. (Skipping tests can end up accidentally hiding real problems.)

Yes, the goal is to have green tests when the UID is too large, but still have as much test coverage as possible.

I will check what other tar implementations do and work on a solution once I find time. (I know, famous last words.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants