From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x732.google.com (mail-qk1-x732.google.com [IPv6:2607:f8b0:4864:20::732]) by sourceware.org (Postfix) with ESMTPS id 5BF393858C60 for ; Tue, 28 Dec 2021 12:22:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5BF393858C60 Received: by mail-qk1-x732.google.com with SMTP id 131so16961564qkk.2 for ; Tue, 28 Dec 2021 04:22:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=M927ZGOZjLSCBBqzf5l1VCkrG8hoE/t4PRM6DMpYLFQ=; b=HJnffhMZjd4PCdVVfrxkSOwpas6Eyg1auD3MezesEuQ+OpvxcSNxBIIVw1e5SPDibx buiExXHvYhgKVpNskY/NCiwAWN1yCG4FoSJtBT/v+hwxeDtFzKnnJtCccGc7jUxYkc1d UDzV5h6YsNldBwmEvAqcvHzbsLgptWOjpVLEtspY6vJTloRBytpoLUP5F8dMIJ3Wl5Br XFmf+k5ouuwrdfJJOCE2zNDJ9FAIZsSMFMntDN6jcz6Ke+aQZLlhyhKv6ZIryURJzIR6 CRJ6n82foWTXqMgvW3rJtB02yonf0e1h/wySoXhp9AZ8798M5V3tgmbUreoTtkNabUDe +/iA== X-Gm-Message-State: AOAM531po7mwilZJZmqdIBWR7lDm/DFE4C769xNveo20cEHWfh3eIEMX +DKl00hpwwhX7cEVtDRU44I8CchLde8QDA== X-Google-Smtp-Source: ABdhPJyqIIfjhZ3DfadVbg1MRhEEPffWoJ2htiQQa08nsIMv6OMCl2KFTJJqopIiSPmY2S6x4OCb+Q== X-Received: by 2002:a05:620a:4251:: with SMTP id w17mr14765285qko.284.1640694166827; Tue, 28 Dec 2021 04:22:46 -0800 (PST) Received: from ?IPV6:2804:431:c7ca:a350:f453:adcd:c68e:4e0d? ([2804:431:c7ca:a350:f453:adcd:c68e:4e0d]) by smtp.gmail.com with ESMTPSA id a28sm11720747qkk.51.2021.12.28.04.22.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 28 Dec 2021 04:22:46 -0800 (PST) Message-ID: <0ed086f7-4e93-ab92-8181-4a41672ded34@linaro.org> Date: Tue, 28 Dec 2021 09:22:44 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH v4] elf: Add _dl_find_object function Content-Language: en-US To: Florian Weimer Cc: libc-alpha@sourceware.org References: <87a6h4zk5z.fsf@oldenburg.str.redhat.com> <87zgor4eyc.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella In-Reply-To: <87zgor4eyc.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Dec 2021 12:22:49 -0000 On 23/12/2021 10:40, Florian Weimer wrote: >>> + /* We may have obtained slightly more space if malloc happened >>> + to provide an over-aligned pointer. */ >>> + result->allocated = (((uintptr_t) (end - ptr) >> >> I think it should ptrdiff_t here. > > I believe uintptr_t results in more efficient code because it is an > unsigned type. I would like to leave it as-is. Alright, my take to use uintptr_t for object counts and pointer difference is in theory easier to detect overflow (as per this gnulib thread [1] and also some changes on the project to provide a idx_t type to handle it transparently). However I think this does not really matter here. > >>> +int >>> +_dl_find_object (void *pc1, struct dl_find_object *result) >>> +{ >>> + uintptr_t pc = (uintptr_t) pc1; >>> + >>> + if (_dlfo_main.map_end == 0) >> >> Maybe __glibc_unlikely here? > > Hmm. Looks like one of the rare occasions where it actually might be > helpful. > >>> + /* Other initially loaded objects. */ >>> + if (pc >= _dlfo_nodelete_mappings->map_start >>> + && pc < _dlfo_nodelete_mappings_end) >>> + { >>> + struct dl_find_object_internal *obj >>> + = _dlfo_lookup (pc, _dlfo_nodelete_mappings, >>> + _dlfo_nodelete_mappings_size); >>> + if (obj != NULL) >>> + { >>> + _dl_find_object_to_external (obj, result); >>> + return 0; >>> + } >>> + /* Fall through to the full search. The kernel may have mapped >>> + the initial mappings with gaps that are later filled by >>> + dlopen with other mappings. */ >>> + } >>> + >>> + /* Handle audit modules, dlopen, dlopen objects. This uses software >> >> Won't audit modules fall in the 'initially loaded objects'? > > Only initially loaded objects in the default namespace are treated as > such. In audit namespaces, we don't know which objects are initally > loaded (and can't be unloaded), and which ones have been dlopen'ed and > could theoretically be dlclose'd by the auditor. I couldn't find a > straightforward way to make the distinction, and deemed it not worth the > additional complexity. Fair enough. > >>> diff --git a/elf/libc-dl_find_object.c b/elf/libc-dl_find_object.c >>> new file mode 100644 >>> index 0000000000..38ea3bc949 >>> --- /dev/null >>> +++ b/elf/libc-dl_find_object.c > >>> +int >>> +_dl_find_object (void *address, struct dl_find_object *result) >>> +{ >>> + return GLRO (dl_find_object) (address, result); >>> +} >> >> Why this function is required? Can we use GLRO (dl_find_object) on >> libc code? > > This function is needed to export _dl_find_object from libc.so.6. This > approach has the advantage that we can patch GLRO (dl_find_object) > during static dlopen. Placing this function in libc also avoids an > annoying linking issue during the glibc build: We do not link with the > libc.so linker script, so some links involving libgcc_s.so fail if > _dl_find_object is only available in ld.so. Ok. > >>> +/* Compare _dl_find_object result at ADDRESS with *EXPECTED. */ >>> +static void >>> +check (void *address, struct dl_find_object *expected, int line) >>> +{ >>> + struct dl_find_object actual; >>> + int ret = _dl_find_object (address, &actual); >>> + if (expected == NULL) >>> + { >>> + if (ret != -1) >>> + { >>> + support_record_failure (); >>> + printf ("%s:%d: unexpected success for %p\n", >>> + __FILE__, line, address); >>> + } >>> + return; >>> + } >> >> Can't we use TEST_VERIFY here? >> >>> + if (ret != 0) >>> + { >>> + support_record_failure (); >>> + printf ("%s:%d: unexpected failure for %p\n", >>> + __FILE__, line, address); >>> + return; >>> + } >>> + >> >> Same as before. >> >>> + if (actual.dlfo_flags != expected->dlfo_flags) >>> + { >>> + support_record_failure (); >>> + printf ("%s:%d: error: %p: flags is %llu, expected %llu\n", >>> + __FILE__, line, address, >>> + actual.dlfo_flags, expected->dlfo_flags); >>> + } >> >> Can't we use TEST_COMPARE and in the following tests? > > I wanted to add more diagnostics. Yeah, I notices it later. Maybe add a new macro, REPORT_FAILURE or something, to avoid the boilerplate code. > >>> +/* Request process termination after 3 seconds. */ >>> +static bool exit_requested; >>> +static void * >>> +exit_thread (void *ignored) >>> +{ >>> + usleep (3 * 100 * 1000); >> >> This will add a 3s, do we really need this large timeout? > > We can lower it, but it makes it less likely the test will find > concurrency issues. Ok, I think we can live with this. > >>> + struct drand48_data state; >>> + srand48_r (1, &state); >>> + while (!__atomic_load_n (&exit_requested, __ATOMIC_RELAXED)) >>> + { >>> + long int idx; >>> + lrand48_r (&state, &idx); >>> + idx %= array_length (temp_objects); >>> + if (temp_objects[idx].link_map == NULL) >>> + { >>> + temp_objects[idx].link_map = xdlopen (temp_objects[idx].soname, >>> + RTLD_NOW); >>> + temp_objects[idx].address = xdlsym (temp_objects[idx].link_map, >>> + temp_objects[idx].symbol); >>> + } >>> + else >>> + { >>> + xdlclose (temp_objects[idx].link_map); >>> + temp_objects[idx].link_map = NULL; >>> + struct dl_find_object dlfo; >>> + int ret = _dl_find_object (temp_objects[idx].address, &dlfo); >>> + if (ret != -1) >> >> Can't you use use TEST_VERIFY_EXIT (ret == 0) here and add debug information >> only for '-d' option? > > See above, I'd like to have the information in the logs in case of > spurious failures, so that we can investigate them. > >>> +static void >>> +check_initial (void) >>> +{ >>> +#ifndef FOR_STATIC >>> + /* Avoid direct reference, which could lead to copy relocations. */ >>> + struct r_debug *debug = xdlsym (NULL, "_r_debug"); >>> + TEST_VERIFY_EXIT (debug != NULL); >>> + char **tzname = xdlsym (NULL, "tzname"); >>> + >>> + /* The main executable has an unnamed link map. */ >>> + struct link_map *main_map = (struct link_map *) debug->r_map; >>> + TEST_COMPARE_STRING (main_map->l_name, ""); >>> + >>> + /* The link map of the dynamic linker. */ >>> + struct link_map *rtld_map = xdlopen (LD_SO, RTLD_LAZY | RTLD_NOLOAD); >>> + TEST_VERIFY_EXIT (rtld_map != NULL); >>> + >>> + /* The link map of libc.so. */ >>> + struct link_map *libc_map = xdlopen (LIBC_SO, RTLD_LAZY | RTLD_NOLOAD); >>> + TEST_VERIFY_EXIT (libc_map != NULL); >> >> I know this an internal test, but maybe use dlinfo or add a wrapper to >> return the link_map on libsupport instead? > > Hmm. Is this really necessary? I can add a helper function to > support/. > > But dlopen returns a void *, so this function will not add any > additional type safety, just a tiny bit of extra documentation > (but searching for “link_map” would already give you places to check if > we ever need to break the dlopen/struct link_map association). It is an glibc test in the end, however it bleed an implementation detail that we will need to possible handle if we ever decide to change it. But it should not really matter in fact because this is unlikely we ever change it. [1] https://lists.gnu.org/archive/html/bug-gnulib/2017-06/msg00009.html