From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x843.google.com (mail-qt1-x843.google.com [IPv6:2607:f8b0:4864:20::843]) by sourceware.org (Postfix) with ESMTPS id C55FC3950C0D for ; Thu, 15 Oct 2020 17:52:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C55FC3950C0D Received: by mail-qt1-x843.google.com with SMTP id j62so16827qtd.0 for ; Thu, 15 Oct 2020 10:52:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:references:from:autocrypt:subject:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=jnVUOO0q1ypsREG1V3Zv/bq6sLxCDrdt+V4lvW1RLeA=; b=N+Qed3y1F00i59K/J0b4f6G+Eehv5ouQ1Gj42bcb1+XsGZldLzb6+dCc0YlZyvxc+D aT6XVpx0Jt6wrjehFs3lAndSsdBwAbo0zKt6NAtPNtLR6nJUbzeszi5549H/kem+FEu3 0JDZQQ7zGXfBsYofUoA5Dk7CTcZVaBuFykKrW65PbsKrYlxpB7sVybfBhP1IC4hacLfl mUVftqsZ3NMxhesg0xfrAD515y8ntavZAsFy1YwDn2CivBjZOOfC8hUSyPbPg3KboF08 l/9zFpdW4ClEvxLUBMdh2dMDe5kKsc9ELvbg1LJ+kA9rnzRF+VYMn+kNGx/jLhKKtCVx d8XA== X-Gm-Message-State: AOAM531HM24zU7jkbeqN5kmrmQvkEg0TDA3a/0/zCya5wl4OEgReRib1 m2I4huteUGP3OHRcw/ZgQpOIezL7erEuAA== X-Google-Smtp-Source: ABdhPJxECP0G0/sGYMdeTBUFIWJ9g90hdI8BCfTEKSqO2affh+nGsc9ZEA46qTduQD2u5ckIICrR0g== X-Received: by 2002:ac8:35a1:: with SMTP id k30mr5259219qtb.387.1602784348945; Thu, 15 Oct 2020 10:52:28 -0700 (PDT) Received: from [192.168.1.4] ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id d56sm190qte.34.2020.10.15.10.52.27 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 15 Oct 2020 10:52:28 -0700 (PDT) To: libc-alpha@sourceware.org References: From: Adhemerval Zanella Autocrypt: addr=adhemerval.zanella@linaro.org; prefer-encrypt=mutual; keydata= mQINBFcVGkoBEADiQU2x/cBBmAVf5C2d1xgz6zCnlCefbqaflUBw4hB/bEME40QsrVzWZ5Nq 8kxkEczZzAOKkkvv4pRVLlLn/zDtFXhlcvQRJ3yFMGqzBjofucOrmdYkOGo0uCaoJKPT186L NWp53SACXguFJpnw4ODI64ziInzXQs/rUJqrFoVIlrPDmNv/LUv1OVPKz20ETjgfpg8MNwG6 iMizMefCl+RbtXbIEZ3TE/IaDT/jcOirjv96lBKrc/pAL0h/O71Kwbbp43fimW80GhjiaN2y WGByepnkAVP7FyNarhdDpJhoDmUk9yfwNuIuESaCQtfd3vgKKuo6grcKZ8bHy7IXX1XJj2X/ BgRVhVgMHAnDPFIkXtP+SiarkUaLjGzCz7XkUn4XAGDskBNfbizFqYUQCaL2FdbW3DeZqNIa nSzKAZK7Dm9+0VVSRZXP89w71Y7JUV56xL/PlOE+YKKFdEw+gQjQi0e+DZILAtFjJLoCrkEX w4LluMhYX/X8XP6/C3xW0yOZhvHYyn72sV4yJ1uyc/qz3OY32CRy+bwPzAMAkhdwcORA3JPb kPTlimhQqVgvca8m+MQ/JFZ6D+K7QPyvEv7bQ7M+IzFmTkOCwCJ3xqOD6GjX3aphk8Sr0dq3 4Awlf5xFDAG8dn8Uuutb7naGBd/fEv6t8dfkNyzj6yvc4jpVxwARAQABtElBZGhlbWVydmFs IFphbmVsbGEgTmV0dG8gKExpbmFybyBWUE4gS2V5KSA8YWRoZW1lcnZhbC56YW5lbGxhQGxp bmFyby5vcmc+iQI3BBMBCAAhBQJXFRpKAhsDBQsJCAcDBRUKCQgLBRYCAwEAAh4BAheAAAoJ EKqx7BSnlIjv0e8P/1YOYoNkvJ+AJcNUaM5a2SA9oAKjSJ/M/EN4Id5Ow41ZJS4lUA0apSXW NjQg3VeVc2RiHab2LIB4MxdJhaWTuzfLkYnBeoy4u6njYcaoSwf3g9dSsvsl3mhtuzm6aXFH /Qsauav77enJh99tI4T+58rp0EuLhDsQbnBic/ukYNv7sQV8dy9KxA54yLnYUFqH6pfH8Lly sTVAMyi5Fg5O5/hVV+Z0Kpr+ZocC1YFJkTsNLAW5EIYSP9ftniqaVsim7MNmodv/zqK0IyDB GLLH1kjhvb5+6ySGlWbMTomt/or/uvMgulz0bRS+LUyOmlfXDdT+t38VPKBBVwFMarNuREU2 69M3a3jdTfScboDd2ck1u7l+QbaGoHZQ8ZNUrzgObltjohiIsazqkgYDQzXIMrD9H19E+8fw kCNUlXxjEgH/Kg8DlpoYJXSJCX0fjMWfXywL6ZXc2xyG/hbl5hvsLNmqDpLpc1CfKcA0BkK+ k8R57fr91mTCppSwwKJYO9T+8J+o4ho/CJnK/jBy1pWKMYJPvvrpdBCWq3MfzVpXYdahRKHI ypk8m4QlRlbOXWJ3TDd/SKNfSSrWgwRSg7XCjSlR7PNzNFXTULLB34sZhjrN6Q8NQZsZnMNs TX8nlGOVrKolnQPjKCLwCyu8PhllU8OwbSMKskcD1PSkG6h3r0AquQINBFcVGkoBEACgAdbR Ck+fsfOVwT8zowMiL3l9a2DP3Eeak23ifdZG+8Avb/SImpv0UMSbRfnw/N81IWwlbjkjbGTu oT37iZHLRwYUFmA8fZX0wNDNKQUUTjN6XalJmvhdz9l71H3WnE0wneEM5ahu5V1L1utUWTyh VUwzX1lwJeV3vyrNgI1kYOaeuNVvq7npNR6t6XxEpqPsNc6O77I12XELic2+36YibyqlTJIQ V1SZEbIy26AbC2zH9WqaKyGyQnr/IPbTJ2Lv0dM3RaXoVf+CeK7gB2B+w1hZummD21c1Laua +VIMPCUQ+EM8W9EtX+0iJXxI+wsztLT6vltQcm+5Q7tY+HFUucizJkAOAz98YFucwKefbkTp eKvCfCwiM1bGatZEFFKIlvJ2QNMQNiUrqJBlW9nZp/k7pbG3oStOjvawD9ZbP9e0fnlWJIsj 6c7pX354Yi7kxIk/6gREidHLLqEb/otuwt1aoMPg97iUgDV5mlNef77lWE8vxmlY0FBWIXuZ yv0XYxf1WF6dRizwFFbxvUZzIJp3spAao7jLsQj1DbD2s5+S1BW09A0mI/1DjB6EhNN+4bDB SJCOv/ReK3tFJXuj/HbyDrOdoMt8aIFbe7YFLEExHpSk+HgN05Lg5TyTro8oW7TSMTk+8a5M kzaH4UGXTTBDP/g5cfL3RFPl79ubXwARAQABiQIfBBgBCAAJBQJXFRpKAhsMAAoJEKqx7BSn lIjvI/8P/jg0jl4Tbvg3B5kT6PxJOXHYu9OoyaHLcay6Cd+ZrOd1VQQCbOcgLFbf4Yr+rE9l mYsY67AUgq2QKmVVbn9pjvGsEaz8UmfDnz5epUhDxC6yRRvY4hreMXZhPZ1pbMa6A0a/WOSt AgFj5V6Z4dXGTM/lNManr0HjXxbUYv2WfbNt3/07Db9T+GZkpUotC6iknsTA4rJi6u2ls0W9 1UIvW4o01vb4nZRCj4rni0g6eWoQCGoVDk/xFfy7ZliR5B+3Z3EWRJcQskip/QAHjbLa3pml xAZ484fVxgeESOoaeC9TiBIp0NfH8akWOI0HpBCiBD5xaCTvR7ujUWMvhsX2n881r/hNlR9g fcE6q00qHSPAEgGr1bnFv74/1vbKtjeXLCcRKk3Ulw0bY1OoDxWQr86T2fZGJ/HIZuVVBf3+ gaYJF92GXFynHnea14nFFuFgOni0Mi1zDxYH/8yGGBXvo14KWd8JOW0NJPaCDFJkdS5hu0VY 7vJwKcyHJGxsCLU+Et0mryX8qZwqibJIzu7kUJQdQDljbRPDFd/xmGUFCQiQAncSilYOcxNU EMVCXPAQTteqkvA+gNqSaK1NM9tY0eQ4iJpo+aoX8HAcn4sZzt2pfUB9vQMTBJ2d4+m/qO6+ cFTAceXmIoFsN8+gFN3i8Is3u12u8xGudcBPvpoy4OoG Subject: Re: [PATCH 22/28] elf: Add extension mechanism to ld.so.cache Message-ID: Date: Thu, 15 Oct 2020 14:52:25 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Thu, 15 Oct 2020 17:52:31 -0000 On 01/10/2020 13:33, Florian Weimer via Libc-alpha wrote: > A previously unused new-format header field is used to record > the address of an extension directory. > > This change adds a demo extension which records the version of > ldconfig which builds a file. Looks good in general, some comments below. > --- > elf/cache.c | 89 +++++++++++++++++++++++++++ > sysdeps/generic/dl-cache.h | 123 ++++++++++++++++++++++++++++++++++++- > 2 files changed, 211 insertions(+), 1 deletion(-) > > diff --git a/elf/cache.c b/elf/cache.c > index e0aa616352..3a02a4070a 100644 > --- a/elf/cache.c > +++ b/elf/cache.c > @@ -15,6 +15,7 @@ > You should have received a copy of the GNU General Public License > along with this program; if not, see . */ > > +#include > #include > #include > #include > @@ -33,6 +34,7 @@ > > #include > #include > +#include > > struct cache_entry > { > @@ -161,6 +163,21 @@ check_new_cache (struct cache_file_new *cache) > error (EXIT_FAILURE, 0, _("Cache file has wrong endianness.\n")); > } > > +/* Print the extension information at the cache at start address > + FILE_BASE, of ltength FILE_SIZE bytes. The new-format cache header s/ltength/length > + is at CACHE, and the file name for diagnostics is CACHE_NAME. */ > +static void > +print_extensions (struct cache_extension_all_loaded *ext) > +{ > + if (ext->sections[cache_extension_tag_generator].base != NULL) > + { > + fputs (_("Cache generated by: "), stdout); > + fwrite (ext->sections[cache_extension_tag_generator].base, 1, > + ext->sections[cache_extension_tag_generator].size, stdout); > + putchar ('\n'); > + } > +} > + Ok. Will be the extension tag data always comprised of ascii printable characters? > /* Print the whole cache file, if a file contains the new cache format > hidden in the old one, print the contents of the new format. */ > void > @@ -250,6 +267,11 @@ print_cache (const char *cache_name) > } > else if (format == 1) > { > + struct cache_extension_all_loaded ext; > + if (!cache_extension_load (cache_new, cache, cache_size, &ext)) > + error (EXIT_FAILURE, 0, > + _("Malformed extension data in cache file %s\n"), cache_name); > + > printf (_("%d libs found in cache `%s'\n"), > cache_new->nlibs, cache_name); > Ok. > @@ -260,6 +282,7 @@ print_cache (const char *cache_name) > cache_new->libs[i].osversion, > cache_new->libs[i].hwcap, > cache_data + cache_new->libs[i].value); > + print_extensions (&ext); > } > /* Cleanup. */ > munmap (cache, cache_size); Ok. > @@ -301,6 +324,45 @@ compare (const struct cache_entry *e1, const struct cache_entry *e2) > return res; > } > > +/* Size of the cache extension directory. All tags are assumed to be > + present. */ > +enum > + { > + cache_extension_size = (offsetof (struct cache_extension, sections) > + + (cache_extension_count > + * sizeof (struct cache_extension_section))) > + }; > + > +/* Write the cache extensions to FD. The extension directory is > + assumed to be located at CACHE_EXTENSION_OFFSET. */ > +static void > +write_extensions (int fd, uint32_t cache_extension_offset) > +{ > + assert ((cache_extension_offset % 4) == 0); Maybe a proper error msg instead of an assert here? > + > + struct cache_extension *ext = xmalloc (cache_extension_size); > + ext->magic = cache_extension_magic; > + ext->count = cache_extension_count; > + > + for (int i = 0; i < cache_extension_count; ++i) > + { > + ext->sections[i].tag = i; > + ext->sections[i].flags = 0; > + } > + > + const char *generator > + = "ldconfig " PKGVERSION RELEASE " release version " VERSION; > + ext->sections[cache_extension_tag_generator].offset > + = cache_extension_offset + cache_extension_size; > + ext->sections[cache_extension_tag_generator].size = strlen (generator); > + > + if (write (fd, ext, cache_extension_size) != cache_extension_size > + || write (fd, generator, strlen (generator)) != strlen (generator)) > + error (EXIT_FAILURE, errno, _("Writing of cache extension data failed")); > + > + free (ext); > +} > + Ok. > /* Save the contents of the cache. */ > void > save_cache (const char *cache_name) > @@ -435,6 +497,25 @@ save_cache (const char *cache_name) > && idx_old < cache_entry_old_count) > file_entries->libs[idx_old] = file_entries->libs[idx_old - 1]; > > + /* Compute the location of the extension directory. This > + implementation puts the directory after the string table. The > + size computation matches the write calls below. The extension > + directory does not exist with format 0, so the value does not > + matter. */ > + uint32_t extension_offset = 0; > + if (opt_format != 2) > + extension_offset += file_entries_size; > + if (opt_format != 0) > + { > + if (opt_format != 2) > + extension_offset += pad; > + extension_offset += file_entries_new_size; > + } Ok, although I think we should be good move the 'opt_format' definition to a proper enumeration. > + extension_offset += total_strlen; > + extension_offset = roundup (extension_offset, 4); /* Provide alignment. */ > + if (opt_format != 0) > + file_entries_new->extension_offset = extension_offset; > + > /* Write out the cache. */ > > /* Write cache first to a temporary file and rename it later. */ Ok. > @@ -473,6 +554,14 @@ save_cache (const char *cache_name) > if (write (fd, strings, total_strlen) != (ssize_t) total_strlen) > error (EXIT_FAILURE, errno, _("Writing of cache data failed")); > > + if (opt_format != 0) > + { > + /* Align file position to 4. */ > + off64_t old_offset = lseek64 (fd, extension_offset, SEEK_SET); > + assert ((unsigned long long int) (extension_offset - old_offset) < 4); Same as before, should we add a proper error in this case? > + write_extensions (fd, extension_offset); > + } > + > /* Make sure user can always read cache file */ > if (chmod (temp_name, S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR)) > error (EXIT_FAILURE, errno, Ok. > diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h > index 1b04211f6b..b154740da9 100644 > --- a/sysdeps/generic/dl-cache.h > +++ b/sysdeps/generic/dl-cache.h > @@ -21,7 +21,9 @@ > > #include > #include > +#include > #include > +#include > > #ifndef _DL_CACHE_DEFAULT_ID > # define _DL_CACHE_DEFAULT_ID 3 > @@ -115,7 +117,11 @@ struct cache_file_new > > uint8_t padding_unsed[3]; /* Not used, for future extensions. */ > > - uint32_t unused[4]; /* Leave space for future extensions > + /* File offset of the extension directory. See struct > + cache_extension below. Must be a multiple of four. */ > + uint32_t extension_offset; > + > + uint32_t unused[3]; /* Leave space for future extensions > and align to 8 byte boundary. */ > struct file_entry_new libs[0]; /* Entries describing libraries. */ > /* After this the string table of size len_strings is found. */ > @@ -134,6 +140,121 @@ cache_file_new_matches_endian (const struct cache_file_new *cache) > } > > Ok. > +/* Randomly chosen magic value, which allows for additional > + consistency verification. */ > +enum { cache_extension_magic = (uint32_t) -358342284 }; > + > +/* Tag values for different kinds of extension sections. Similar to > + SHT_* constants. */ > +enum cache_extension_tag > + { > + /* Array of bytes containing the glibc version that generated this > + cache file. */ > + cache_extension_tag_generator, > + > + /* Total number of known cache extension tags. */ > + cache_extension_count > + }; > + > +/* Element in the array following struct cache_extension. Similar to > + an ELF section header. */ > +struct cache_extension_section > +{ > + /* Type of the extension section. A enum cache_extension_tag value. */ > + uint32_t tag; > + > + /* Extension-specific flags. Currently generated as zero. */ > + uint32_t flags; > + > + /* Offset from the start of the file for the data in this extension > + section. Specific extensions can have alignment constraints. */ > + uint32_t offset; > + > + /* Length in bytes of the extension data. Specific extensions may > + have size requirements. */ > + uint32_t size; > +}; Ok. > + > +/* The extension directory in the cache. An array of struct > + cache_extension_section entries. */ > +struct cache_extension > +{ > + uint32_t magic; /* Always cache_extension_magic. */ > + uint32_t count; /* Number of following entries. */ > + > + /* count section descriptors of type struct cache_extension_section > + follow. */ > + struct cache_extension_section sections[]; > +}; > + Ok. > +/* A relocated version of struct cache_extension_section. */ > +struct cache_extension_loaded > +{ > + /* Address and size of this extension section. base is NULL if the > + section is missing from the file. */ > + const void *base; > + size_t size; > + > + /* Flags from struct cache_extension_section. */ > + uint32_t flags; > +}; > + > +/* All supported extension sections, relocated. Filled in by > + cache_extension_load below. */ > +struct cache_extension_all_loaded > +{ > + struct cache_extension_loaded sections[cache_extension_count]; > +}; > + Ok. > +static bool __attribute__ ((unused)) Maybe use inline and let the compiler decide? Or the function is really duplicate in a lot of places? > +cache_extension_load (const struct cache_file_new *cache, > + const void *file_base, size_t file_size, > + struct cache_extension_all_loaded *loaded) > +{ > + memset (loaded, 0, sizeof (*loaded)); > + if (cache->extension_offset == 0) > + /* No extensions present. This is not a format error. */ > + return true; > + if ((cache->extension_offset % 4) != 0) > + /* Extension offset is misaligned. */ > + return false; > + size_t size_tmp; > + if (__builtin_add_overflow (cache->extension_offset, > + sizeof (struct cache_extension), &size_tmp) > + || size_tmp > file_size) > + /* Extension extends beyond the end of the file. */ > + return false; > + const struct cache_extension *ext = file_base + cache->extension_offset; Maybe we should add an alignment check for 'file_base' as well (to avoid unaligned struct member deference)? > + if (ext->magic != cache_extension_magic) > + return false; > + if (__builtin_mul_overflow (ext->count, > + sizeof (struct cache_extension_section), > + &size_tmp) > + || __builtin_add_overflow (cache->extension_offset > + + sizeof (struct cache_extension), size_tmp, > + &size_tmp) > + || size_tmp > file_size) > + /* Extension array extends beyond the end of the file. */ > + return false; > + for (uint32_t i = 0; i < ext->count; ++i) > + { > + if (__builtin_add_overflow (ext->sections[i].offset, > + ext->sections[i].size, &size_tmp) > + || size_tmp > file_size) > + /* Extension data extends beyond the end of the file. */ > + return false; > + > + uint32_t tag = ext->sections[i].tag; > + if (tag >= cache_extension_count) > + /* Tag is out of range and unrecognized. */ > + continue; > + loaded->sections[tag].base = file_base + ext->sections[i].offset; > + loaded->sections[tag].size = ext->sections[i].size; > + loaded->sections[tag].flags = ext->sections[i].flags; > + } > + return true; > +} > + > /* Used to align cache_file_new. */ > #define ALIGN_CACHE(addr) \ > (((addr) + __alignof__ (struct cache_file_new) -1) \ > Ok.