#733274 libeatmydata: eatmydata() should return -ELIBACC instead of -EFAULT if it can't find open()

#733274#5
Date:
2013-12-27 23:35:19 UTC
From:
To:
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.

#733274#10
Date:
2016-10-12 18:38:13 UTC
From:
To:
[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?

#733274#15
Date:
2016-10-23 00:17:43 UTC
From:
To:
 
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.

#733274#20
Date:
2024-12-04 06:38:12 UTC
From:
To:
[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