#733274 libeatmydata: eatmydata() should return -ELIBACC instead of -EFAULT if it can't find open() #733274
- Package:
- libeatmydata
- Source:
- libeatmydata
- Submitter:
- Klee Dienes
- Date:
- 2024-12-04 06:36:02 UTC
- Severity:
- normal
- Tags:
This is related to #702711, but slightly different. EFAULT is confusing to the naive user of libeatmydata, who sees only a call to open() returning an errno of EFAULT and is therefore tempted to try to figure out what is wrong with his or her pointer (not realizing that open() has been replaced). ELIBACC would be closer to the truth of what has gone wrong and a useful clue that shared-library shenanigans are involved.
[Klee Dienes 2013-12-27] According to the POSIX specification, ELIBACC is not in the list of expected errors from open(). See <URL: http://pubs.opengroup.org/onlinepubs/009695399/functions/open.html > for the full specification. Can you provide some example code explaining what kind of error you believe should cause open() to return ELIBACC?
You make a good point about ELIBACC not being in the list of errors from open(). But then again, neither is EFAULT. The point I am trying to express is that open() is being called with valid parameters. But because libeatmydata is not properly initialized, open() is returning EFAULT even though it has been called with valid parameters, which is confusing to the programmer. I think this scenario has mostly been addressed by the fix in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=702711. (I suspect but am not certain that there is still a brief race condition in eatmydata_init()). But I also think that even so, in whatever unlikely circumstance that might lead to the EFAULT case getting returned to a calling program, ELIBACC would be a better choice. EFAULT implies a bad address; ELIBACC at least gives me a clue that there is something strange going on with shared libraries. Or maybe in this case EDEADLOCK since I think this can only occur if two threads call open() simultaneously? Or perhaps we could add a mutex around eatmydata_init() and eliminate the possibility of returning EFAULT at all? I'd be happy to submit a patch for that if you think it's a good approach. The patch in http://bazaar.launchpad.net/~mapreri/libeatmydata/drop-unn eeded-include/revision/99 has made it harder to come up with an example that would return EFAULT (ELIBACC). I'd be willing to construct one if you would find it helpful. Thanks for taking the time to look at this.
[Klee Dienes]
Good point. In that case, I guess it do not make much difference
which non-standard fault is returned, and ELIBACC is just as good as
EFAULT. Here is a draft patch to consider for the package maintainer
and upstream.
diff --git a/debian/patches/open-elibacc.patch b/debian/patches/open-elibacc.patch
new file mode 100644
index 0000000..c68a081
--- /dev/null
+++ b/debian/patches/open-elibacc.patch
@@ -0,0 +1,28 @@
+Description: Use ELIBACC instad of EFAULT when open() fail.
+ This might make it less confusing when a race condition cause
+ eatmydata to fail.
+Author: Petter Reinholdtsen <pere@debian.org>
+Bug-Debian: https://bugs.debian.org/733274
+Forwarded: no
+Last-Update: 2024-12-04
+---
+--- libeatmydata-131.orig/libeatmydata/libeatmydata.c
++++ libeatmydata-131/libeatmydata/libeatmydata.c
+@@ -183,7 +183,7 @@ int LIBEATMYDATA_API open(const char* pa
+ /* If we get called recursively during initialization (which should
+ * be rare but might happen), just fail. */
+ if (init_running > 0) {
+- errno = EFAULT;
++ errno = ELIBACC;
+ return -1;
+ }
+
+@@ -221,7 +221,7 @@ int LIBEATMYDATA_API open64(const char*
+ /* If we get called recursively during initialization (which should
+ * be rare but might happen), just fail. */
+ if (init_running > 0) {
+- errno = EFAULT;
++ errno = ELIBACC;
+ return -1;
+ }
+
diff --git a/debian/patches/series b/debian/patches/series
index 6298cd5..768035e 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -3,3 +3,4 @@ tests.patch
drop-debian-stuff.patch
test-no-distcheck.path
t64.patch
+open-elibacc.patch