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

options.c: Fix segv if poptGetContext returns NULL #728

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sahlberg
Copy link

@sahlberg sahlberg commented Jan 30, 2025

If poptGetContext returns NULL, perhaps due to OOM, a NULL pointer is passed into poptReadDefaultConfig() which in turns SEGVs when trying to dereference it.

This was found using https://github.com/sahlberg/malloc-fail-tester.git

$ ./test_malloc_failure.sh rsync -Pav crash crosh

Need to test 4869
4891 allocations for rsync -Pav crash crosh
./test_malloc_failure.sh: line 9: 549305 Segmentation fault      (core dumped) ALLOC_FAIL=${IDX} LD_PRELOAD=./ld_alloc.so $PROGRAM $@ > /dev/null 2>&1
Crash at allocation #0
           PID: 549305 (rsync)
           UID: 1000 (sahlberg)
           GID: 1000 (sahlberg)
        Signal: 11 (SEGV)
     Timestamp: Thu 2025-01-30 13:29:58 AEST (721ms ago)
  Command Line: rsync -Pav crash crosh
    Executable: /usr/bin/rsync
 Control Group: /user.slice/user-1000.slice/[email protected]/app.slice/app-org.gnome.Terminal.slice/vte-spawn-90cea7ac-a067-4bff-9d0e-a8ef44417922.scope
          Unit: [email protected]
     User Unit: vte-spawn-90cea7ac-a067-4bff-9d0e-a8ef44417922.scope
         Slice: user-1000.slice
     Owner UID: 1000 (sahlberg)
       Boot ID: 51235d2adce14917828bb9d62b9bd051
    Machine ID: 94b9e1dd726e4cc9a0fb0dc93b9a801a
      Hostname: fedora
       Storage: /var/lib/systemd/coredump/core.rsync.1000.51235d2adce14917828bb9d62b9bd051.549305.1738207798000000.zst (present)
  Size on Disk: 103.8K
       Package: rsync/3.3.0-1.fc40
      build-id: fd1891af7f6287d01d804c8932b4847ed95206f5
       Message: Process 549305 (rsync) of user 1000 dumped core.

                Module libz.so.1 from rpm zlib-ng-2.1.7-2.fc40.x86_64
                Module libattr.so.1 from rpm attr-2.5.2-3.fc40.x86_64
                Module libcrypto.so.3 from rpm openssl-3.2.2-3.fc40.x86_64
                Module libxxhash.so.0 from rpm xxhash-0.8.3-1.fc40.x86_64
                Module libzstd.so.1 from rpm zstd-1.5.6-1.fc40.x86_64
                Module liblz4.so.1 from rpm lz4-1.9.4-6.fc40.x86_64
                Module libpopt.so.0 from rpm popt-1.19-6.fc40.x86_64
                Module libacl.so.1 from rpm acl-2.3.2-1.fc40.x86_64
                Module rsync from rpm rsync-3.3.0-1.fc40.x86_64
                Stack trace of thread 549305:
                #0  0x00007f5b097f2f67 poptReadDefaultConfig (libpopt.so.0 + 0xaf67)
                #1  0x000055bd526684d2 parse_arguments (rsync + 0x374d2)
                #2  0x000055bd52637f77 main (rsync + 0x6f77)
                #3  0x00007f5b09039088 __libc_start_call_main (libc.so.6 + 0x2a088)
                #4  0x00007f5b0903914b __libc_start_main@@GLIBC_2.34 (libc.so.6 + 0x2a14b)
                #5  0x000055bd5263b5b5 _start (rsync + 0xa5b5)
                ELF object binary architecture: AMD x86-64
NUM_CRASHES:1

If poptGetContext returns NULL, perhaps due to OOM,
a NULL pointer is passed into poptReadDefaultConfig()
which in turns SEGVs when trying to dereference it.

This was found using https://github.com/sahlberg/malloc-fail-tester.git
$ ./test_malloc_failure.sh rsync -Pav crash crosh

Signed-off-by: Ronnie Sahlberg <[email protected]>
@@ -1369,6 +1369,10 @@ int parse_arguments(int *argc_p, const char ***argv_p)
/* TODO: Call poptReadDefaultConfig; handle errors. */

pc = poptGetContext(RSYNC_NAME, argc, argv, long_options, 0);
if (pc == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while correct, checking for NULL is also a layering violation, as the object returned is opaque.

doing instead if (!pc) as done in the cleanup label should be better IMHO, but adding something to the documentation of poptGetContext() that validates that use case even better.

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

Successfully merging this pull request may close these issues.

2 participants