Re: [PATCH] PR29022: 000-permissions files cause problems for backups
Hi Aaron,
On Fri, 2022-04-08 at 19:37 -0400, Aaron Merey via Elfutils-devel
wrote:
> I've revised this patch so that the negative-hit file's mtime is used
> to calculate time since last download attempt instead of the cache_miss_s
> file. I've also added a check for old 000-permission files so that they
> are unlinked immediately if found.
>
> Aaron
>
> ---
> PR29022: 000-permissions files cause problems for backups
>
> 000-permission files currently used for negative caching can cause
> permission problems for some backup software and disk usage checkers.
>
> Fix this by using empty files to for negative caching instead.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=29022
>
> Signed-off-by: Aaron Merey
Please also mention the fix for the mtime/stat/cache_miss_s issue in
the commit message.
> ---
> debuginfod/ChangeLog | 5 +
> debuginfod/debuginfod-client.c| 94 -
> --
> tests/ChangeLog | 5 +
> tests/Makefile.am | 4 +-
> tests/run-debuginfod-federation-link.sh | 4 +-
> tests/run-debuginfod-federation-metrics.sh| 4 +-
> tests/run-debuginfod-federation-sqlite.sh | 4 +-
> ...on.sh => run-debuginfod-negative-cache.sh} | 6 +-
> tests/run-debuginfod-tmp-home.sh | 2 +-
> 9 files changed, 81 insertions(+), 47 deletions(-)
> rename tests/{run-debuginfod-000-permission.sh => run-debuginfod-
> negative-cache.sh} (92%)
>
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 578d951d..6c2edbdf 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-04-05 Aaron Merey
> +
> + * debuginfod-client.c: Represent negative caching with empty
> files
> + instead of 000-permission files.
A ChangeLog entry really should mention the how things were
changed/fixed, not just restate the what, which is what the commit
message is for. I admit that I am probably the only person still seeing
value in ChangeLog entries, but if we are doing them I would suggest
something like:
* debuginfod-client.c (debuginfod_query_server):
Drop st_mode check. Add st_size > 0 check.
Save target_mtime before calling
debuginfod_config_cache. unlink target_cache_path
on EACCESS. Create target_cache_path with DEFFILEMODE.
> @@ -767,42 +767,66 @@ debuginfod_query_server (debuginfod_client *c,
>if (rc != 0)
> goto out;
>
> - struct stat st;
> - /* Check if the file exists and it's of permission 000; must check
> - explicitly rather than trying to open it first (PR28240). */
> - if (stat(target_cache_path, &st) == 0
> - && (st.st_mode & 0777) == 0)
> + /* Check if the target is already in the cache. */
> + int fd = open(target_cache_path, O_RDONLY);
> + if (fd >= 0)
> {
> - time_t cache_miss;
> -
> - rc = debuginfod_config_cache(cache_miss_path,
> cache_miss_default_s, &st);
> - if (rc < 0)
> -goto out;
> + struct stat st;
> + if (fstat(fd, &st) != 0)
> +{
> + close (fd);
> + rc = -errno;
> + goto out;
> +}
Call close after setting rc = -errno. close might also set errno,
but we want to make sure to report the original errno that fstat
returned (close probably will report the same errno, but might not,
it might also set errno to 0 which is super confusing).
> - cache_miss = (time_t)rc;
> - if (time(NULL) - st.st_mtime <= cache_miss)
> + /* If the file is non-empty, then we are done. */
> + if (st.st_size > 0)
> {
> - rc = -ENOENT;
> - goto out;
> - }
> + if (path != NULL)
> +{
> + *path = strdup(target_cache_path);
> + if (*path == NULL)
> +{
> + close (fd);
> + rc = -errno;
> + goto out;
> +}
Likewise.
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index c195f9f7..91ce1ffb 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-04-05 Aaron Merey
> +
> + * run-debuginfod-000-permissions.sh: Delete.
> + * run-debuginfod-negative-cache.sh: New test.
Missing:
* Makefile.am (TESTS): Remove run-debuginfod-000-permission.sh
and add run-debuginfod-negative-cache.sh.
(EXTRA_DIST): Likewise.
* run-debuginfod-federation-link.sh: Update comments about
negative-hit file.
* run-debuginfod-federation-metrics.sh: Likewise.
* run-debuginfod-federation-sqlite.sh: Likewise.
* run-debuginfod-tmp-home.sh: Likewise.
> -if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` !=
> "--" ]; then
> - echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is readable"
> +if [ `stat -c "%s" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != 0 ]; then
> + echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debugin
[Bug general/29048] Symbols DW_SECT_INFO, DW_SECT_ABBREV, DW_SECT_STR_OFFSETS aren't defined
https://sourceware.org/bugzilla/show_bug.cgi?id=29048 Mark Wielaard changed: What|Removed |Added CC||mark at klomp dot org --- Comment #1 from Mark Wielaard --- I can add those macro constants to dwarf.h, but elfutils libdw at the moment doesn't handle .dwp, DWARF package files. How does folly use these? -- You are receiving this mail because: You are on the CC list for the bug.
[Bug general/29048] Symbols DW_SECT_INFO, DW_SECT_ABBREV, DW_SECT_STR_OFFSETS aren't defined
https://sourceware.org/bugzilla/show_bug.cgi?id=29048 --- Comment #2 from yuri at tsoft dot com --- They are only used as constants, -- You are receiving this mail because: You are on the CC list for the bug.
[COMMITTED] libdw: Add DWARF5 package file section identifiers, DW_SECT_*
This only adds the constants. There is no handling of DWARF
package file (dwp) files for now.
https://sourceware.org/bugzilla/show_bug.cgi?id=29048
Signed-off-by: Mark Wielaard
---
libdw/ChangeLog | 5 +
libdw/dwarf.h | 13 +
2 files changed, 18 insertions(+)
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index ca742e6b..38f3a7e2 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,8 @@
+2022-04-13 Mark Wielaard
+
+ * dwarf.h: Add DWARF5 package file section identifiers,
+ DW_SECT_*.
+
2021-10-20 John M Mellor-Crummey
* dwarf_linecontext.c: New file.
diff --git a/libdw/dwarf.h b/libdw/dwarf.h
index 3ce7f236..c961bc36 100644
--- a/libdw/dwarf.h
+++ b/libdw/dwarf.h
@@ -934,6 +934,19 @@ enum
DW_LLE_GNU_start_length_entry = 0x3
};
+/* DWARF5 package file section identifiers. */
+enum
+ {
+DW_SECT_INFO = 1,
+/* Reserved = 2, */
+DW_SECT_ABBREV = 3,
+DW_SECT_LINE = 4,
+DW_SECT_LOCLISTS = 5,
+DW_SECT_STR_OFFSETS = 6,
+DW_SECT_MACRO = 7,
+DW_SECT_RNGLISTS = 8,
+ };
+
/* DWARF call frame instruction encodings. */
enum
--
2.18.4
[Bug general/29048] Symbols DW_SECT_INFO, DW_SECT_ABBREV, DW_SECT_STR_OFFSETS aren't defined
https://sourceware.org/bugzilla/show_bug.cgi?id=29048 Mark Wielaard changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED --- Comment #3 from Mark Wielaard --- Added just the constants: commit 399b55a75830f1854c8da9f29282810e82f270b6 Author: Mark Wielaard Date: Wed Apr 13 17:23:57 2022 +0200 libdw: Add DWARF5 package file section identifiers, DW_SECT_* This only adds the constants. There is no handling of DWARF package file (dwp) files for now. https://sourceware.org/bugzilla/show_bug.cgi?id=29048 Signed-off-by: Mark Wielaard -- You are receiving this mail because: You are on the CC list for the bug.
☠ Buildbot (Wildebeest Builder): elfutils - failed test (failure) (master)
A new failure has been detected on builder elfutils-centos-x86_64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/1/builds/932 Build state: failed test (failure) Revision: 399b55a75830f1854c8da9f29282810e82f270b6 Worker: centos-x86_64 Build Reason: (unknown) Blamelist: Mark Wielaard Steps: - 0: worker_preparation ( success ) - 1: set package name ( success ) - 2: git checkout ( success ) Logs: - stdio: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/2/logs/stdio - 3: autoreconf ( success ) Logs: - stdio: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/3/logs/stdio - 4: configure ( success ) Logs: - stdio: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/4/logs/stdio - 5: get version ( success ) Logs: - stdio: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/5/logs/stdio - property changes: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/5/logs/property_changes - 6: shell ( success ) Logs: - stdio: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/6/logs/stdio - 7: make ( warnings ) Logs: - stdio: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/7/logs/stdio - warnings (2): https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/7/logs/warnings__2_ - 8: make check ( failure ) Logs: - stdio: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/8/logs/stdio - test-suite.log: https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/8/logs/test-suite_log
Re: ☠ Buildbot (Wildebeest Builder): elfutils - failed test (failure) (master)
Hi,
On Wed, Apr 13, 2022 at 05:21:03PM +, build...@builder.wildebeest.org wrote:
> A new failure has been detected on builder elfutils-centos-x86_64 while
> building elfutils.
>
> Full details are available at:
> https://builder.wildebeest.org/buildbot/#builders/1/builds/932
>
> Build state: failed test (failure)
> Revision: 399b55a75830f1854c8da9f29282810e82f270b6
> Worker: centos-x86_64
> Build Reason: (unknown)
> Blamelist: Mark Wielaard
>
> Steps:
> [...]
> - 8: make check ( failure )
> Logs:
> - stdio:
> https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/8/logs/stdio
> - test-suite.log:
> https://builder.wildebeest.org/buildbot/#builders/1/builds/932/steps/8/logs/test-suite_log
Hmmm, this seems a little random. The change was just adding some
(unused) constants to dwarf.h. The log says:
command timed out: 1200 seconds without output running ['make', 'check',
'-j4'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1925.591587
It looks like run-debuginfod-federation-sqlite.sh is missing.
Looking at the buildbot worker the end of
run-debuginfod-federation-sqlite.sh.log is:
+ mvalue=2
+ '[' -z 2 ']'
+ echo 'metric thread_work_total{role="groom"}: 2'
metric thread_work_total{role="groom"}: 2
+ '[' 2 -eq 2 ']'
+ break
+ '[' 18 -eq 0 ']'
+ curl -s http://127.0.0.1:9112/buildid/beefbeefbeefd00dd00d/debuginfo
+ curl -s http://127.0.0.1:9112/metrics
+ grep 'error_count.*sqlite'
error_count{sqlite3="database disk image is malformed"} 6
error_count{sqlite3="file is encrypted or is not a database"} 1
+ kill -INT 28184 28371
+ wait 28184 28371
Which seems to correspond to this part in run-debuginfod-federation-sqlite.sh
# Corrupt the sqlite database and get debuginfod to trip across its errors
curl -s http://127.0.0.1:$PORT1/metrics | grep 'sqlite3.*reset'
dd if=/dev/zero of=$DB bs=1 count=1
# trigger some random activity that's Sure to get sqlite3 upset
kill -USR1 $PID1
wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
wait_ready $PORT1 'thread_busy{role="scan"}' 0
kill -USR2 $PID1
wait_ready $PORT1 'thread_work_total{role="groom"}' 2
curl -s http://127.0.0.1:$PORT1/buildid/beefbeefbeefd00dd00d/debuginfo >
/dev/null || true
curl -s http://127.0.0.1:$PORT1/metrics | grep 'error_count.*sqlite'
# Run the tests again without the servers running. The target file should
# be found in the cache.
kill -INT $PID1 $PID2
wait $PID1 $PID2
So maybe corruptin the sqlite database prevents a proper shutdown of
the debuginfod process?
Cheers,
Mark
Re: ☠ Buildbot (Wildebeest Builder): elfutils - failed test (failure) (master)
Hi - > So maybe corruptin the sqlite database prevents a proper shutdown of > the debuginfod process? Yeah, that test is a bit blunt and unpredictable. (We're literally overwriting a piece of the database file that the process has open and has random parts in cache.) I'm thinking this is not worth testing. - FChE
Re: [PATCH] PR29022: 000-permissions files cause problems for backups
Hi Mark, Thanks for the review. I fixed the issues you pointed out (good catch re. setting rc before closing fd) and pushed as commit 8b568fdea8 with Tested-by: Milian Wolff added. Milian, thanks again for taking a look at this. Aaron
[Bug debuginfod/29022] 000-permissions files cause problems for backups
https://sourceware.org/bugzilla/show_bug.cgi?id=29022 Aaron Merey changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #5 from Aaron Merey --- A fix has been pushed as: commit 8b568fdea8e1baea3d68cc38dec587e4c9219276 Author: Aaron Merey Date: Fri Apr 8 19:37:11 2022 -0400 PR29022: 000-permissions files cause problems for backups 000-permission files currently used for negative caching can cause permission problems for some backup software and disk usage checkers. Fix this by using empty files for negative caching instead. Also use each empty file's mtime to determine the time since last download attempt instead of the cache_miss_s file's mtime. https://sourceware.org/bugzilla/show_bug.cgi?id=29022 Tested-by: Milian Wolff Signed-off-by: Aaron Merey -- You are receiving this mail because: You are on the CC list for the bug.
