derickr / timelib

Timelib is a timezone and date/time library that can calculate local time, convert between timezones and parse textual descriptions of date/time information.
MIT License
122 stars 68 forks source link

Loading timezone data from an invalid directory causes a SEGV #158

Closed albymassari closed 2 days ago

albymassari commented 3 days ago

It can be reproduced using the enumerate-timezone test:

~/timelib$ tests/enumerate-timezones tests/c/files/NonContinuous
parse_tz.c:614:15: runtime error: member access within null pointer of type 'const struct timelib_tzdb'
AddressSanitizer:DEADLYSIGNAL
=================================================================
==247414==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0xbd4d66bb7e44 bp 0xffffc210d5a0 sp 0xffffc210d5a0 T0)
==247414==The signal is caused by a READ memory access.
==247414==Hint: address points to the zero page.
    #0 0xbd4d66bb7e44 in timelib_timezone_identifiers_list /home/ubuntu/timelib/parse_tz.c:614
    #1 0xbd4d66babcc0 in main tests/enumerate-timezones.c:44
    #2 0xf65972a684c0 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #3 0xf65972a68594 in __libc_start_main_impl ../csu/libc-start.c:360
    #4 0xbd4d66baba6c in _start (/home/ubuntu/timelib/tests/enumerate-timezones+0x5ba6c) (BuildId: 70fe6083f5ab5d8bff6ac292216c014266c6c8f1)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/ubuntu/timelib/parse_tz.c:614 in timelib_timezone_identifiers_list
==247414==ABORTING

It also happens for missing folders

~/timelib$ tests/enumerate-timezones tests/c/files/missing
parse_tz.c:614:15: runtime error: member access within null pointer of type 'const struct timelib_tzdb'
AddressSanitizer:DEADLYSIGNAL
=================================================================
==247421==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0xc1b4b0367e44 bp 0xffffe12ec450 sp 0xffffe12ec450 T0)
==247421==The signal is caused by a READ memory access.
==247421==Hint: address points to the zero page.
    #0 0xc1b4b0367e44 in timelib_timezone_identifiers_list /home/ubuntu/timelib/parse_tz.c:614
    #1 0xc1b4b035bcc0 in main tests/enumerate-timezones.c:44
    #2 0xe130f2e684c0 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #3 0xe130f2e68594 in __libc_start_main_impl ../csu/libc-start.c:360
    #4 0xc1b4b035ba6c in _start (/home/ubuntu/timelib/tests/enumerate-timezones+0x5ba6c) (BuildId: 70fe6083f5ab5d8bff6ac292216c014266c6c8f1)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/ubuntu/timelib/parse_tz.c:614 in timelib_timezone_identifiers_list
==247421==ABORTING
albymassari commented 3 days ago

If the SEGV is avoided, there is also a leak in the cleanup code

        count = timelib_scandir(name, &ents, index_filter, timelib_alphasort);
        if (count == -1) {
            timelib_free(dirstack);
            timelib_free(db_index);
            return -errno;
        }

The memory holding the dirstack is freed, but the strings duplicated with strdup held in that stack are not freed (there is at least one, 1-byte long, held in the "top" variable)

~/timelib$ tests/enumerate-timezones tests/c/files/missing
FAIL: Cannot load timezone info

=================================================================
==247796==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0xff5e174da5f4 in strdup ../../../../src/libsanitizer/asan/asan_interceptors.cpp:578
    #1 0xae3d250b054c in create_zone_index /home/ubuntu/timelib/parse_zoneinfo.c:239
    #2 0xae3d250b15b0 in timelib_zoneinfo /home/ubuntu/timelib/parse_zoneinfo.c:337
    #3 0xae3d2509bd6c in main tests/enumerate-timezones.c:41
    #4 0xff5e16a684c0 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #5 0xff5e16a68594 in __libc_start_main_impl ../csu/libc-start.c:360
    #6 0xae3d2509bb2c in _start (/home/ubuntu/timelib/tests/enumerate-timezones+0x5bb2c) (BuildId: 7fc62fbc0b8989a10a54e05bbf87ae3344dac6bb)

SUMMARY: AddressSanitizer: 1 byte(s) leaked in 1 allocation(s).
derickr commented 2 days ago

Hi,

I think this segfault happens not if there is an invalid file, but if there is an invalid directory.

For example, the following works "fine":

OK:   Casablanca_AmazonLinux1
OK:   New_York_Fat
OK:   New_York_mod_Full_Year_DST
OK:   New_York_Slim
OK:   Nicosia_TZif4
FAIL: NonContinuous: [2] Corrupt tzfile: The transitions in the file don't always increase
OK:   Nuuk_AmazonLinux1

When you pass in a non-directory (such as the file tests/c/files/NonContinuous), then indeed enumerate-timezones segfaults. Like with: ./tests/enumerate-timezones tests/c/files/NonContinuous

$ ./tests/enumerate-timezones tests/c/files

I have now used your changes in PR #159 to create two commits that I have merged into the v2022 and master branches.

Thanks! Derick