From 64edb09accf11480adf9452b317bfa25129a09be Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sun, 24 Apr 2022 21:30:44 -0700 Subject: [PATCH] sfio: remove 'SF_MTSAFE' MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This constant was performing two overlapping roles: 1. A flag callers could use to indicate they anticipated calling into sfio functions with the same object from multiple threads. 2. A flag used by the library itself to indicate whether thread-safety was available. To deal with both macros having the same name, sfhdr.h was forcing the value seen internally to zero. This mechanism has numerous shortcomings: 1. It was possible for a caller to use the external value of `SF_MTSAFE` (010000) that, thanks to the overriding described above, sfio itself would fail to understand. 2. Including sfio.h within sfio sources without including sfhdr.h was a latent foot gun that would result in part of sfio believing the `SF_MTSAFE` flag _did_ have meaning. 3. sfio functions are not thread-safe, with or without `SF_MTSAFE` defined, and regardless of what value it has. To take an example to illustrate 3, `sfopen` does something like double-checked locking¹ to anticipate another thread modifying `f` in-between its first check and the point at which it calls `SFMTXSTART`. Except `SFMTXSTART` is not a lock. There is no synchronization point here, and in fact an optimizing compiler will simply coalesce both the first and second checks into one. It is not clear to me whether this code was written when multicore machines were uncommon (though this design is also unsafe on a unicore machine) or if `SFMTXSTART` is some kind of placeholder that was meant to one day grow a locking mechanism. It seems safer and more honest to remove this constant, and with it the implication that thread-safety is toggle-able. Migrating to C stdio (#1998) will render all of this moot as it is thread-safe by default. ¹ https://en.wikipedia.org/wiki/Double-checked_locking --- lib/sfio/sfextern.c | 9 +++------ lib/sfio/sfhdr.h | 7 ++----- lib/sfio/sfio.h | 1 - lib/sfio/sfopen.c | 2 -- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/lib/sfio/sfextern.c b/lib/sfio/sfextern.c index fcf4f3992..bf7ef301a 100644 --- a/lib/sfio/sfextern.c +++ b/lib/sfio/sfextern.c @@ -25,12 +25,9 @@ ssize_t _Sfi = -1; #define SFMTXOUT (0) #define SFMTXERR (0) -Sfio_t _Sfstdin = SFNEW(NULL, -1, 0, (SF_READ | SF_STATIC | SF_MTSAFE), NULL, - SFMTXIN); -Sfio_t _Sfstdout = SFNEW(NULL, -1, 1, (SF_WRITE | SF_STATIC | SF_MTSAFE), NULL, - SFMTXOUT); -Sfio_t _Sfstderr = SFNEW(NULL, -1, 2, (SF_WRITE | SF_STATIC | SF_MTSAFE), NULL, - SFMTXERR); +Sfio_t _Sfstdin = SFNEW(NULL, -1, 0, (SF_READ | SF_STATIC), NULL, SFMTXIN); +Sfio_t _Sfstdout = SFNEW(NULL, -1, 1, (SF_WRITE | SF_STATIC), NULL, SFMTXOUT); +Sfio_t _Sfstderr = SFNEW(NULL, -1, 2, (SF_WRITE | SF_STATIC), NULL, SFMTXERR); Sfio_t *sfstdin = &_Sfstdin; Sfio_t *sfstdout = &_Sfstdout; diff --git a/lib/sfio/sfhdr.h b/lib/sfio/sfhdr.h index 745d29e9f..0700a4f31 100644 --- a/lib/sfio/sfhdr.h +++ b/lib/sfio/sfhdr.h @@ -57,9 +57,6 @@ extern "C" { #include #include -#undef SF_MTSAFE /* no need to worry about thread-safety */ -#define SF_MTSAFE 0 - #define SFMTXSTART(f,v) { if(!f) return(v); } #define SFMTXRETURN(f,v) { return(v); } @@ -360,8 +357,8 @@ extern "C" { /* lock/open a stream */ #define SFMODE(f,l) ((f)->mode & ~(SF_RV|SF_RC|((l) ? SF_LOCK : 0)) ) #define SFLOCK(f,l) (void)((f)->mode |= SF_LOCK, (f)->endr = (f)->endw = (f)->data) -#define _SFOPENRD(f) ((f)->endr = ((f)->flags&SF_MTSAFE) ? (f)->data : (f)->endb) -#define _SFOPENWR(f) ((f)->endw = ((f)->flags&(SF_MTSAFE|SF_LINE)) ? (f)->data : (f)->endb) +#define _SFOPENRD(f) ((f)->endr = (f)->endb) +#define _SFOPENWR(f) ((f)->endw = ((f)->flags&SF_LINE) ? (f)->data : (f)->endb) #define _SFOPEN(f) ((f)->mode == SF_READ ? _SFOPENRD(f) : \ (f)->mode == SF_WRITE ? _SFOPENWR(f) : \ ((f)->endw = (f)->endr = (f)->data) ) diff --git a/lib/sfio/sfio.h b/lib/sfio/sfio.h index b5bf21d57..3d6fb496a 100644 --- a/lib/sfio/sfio.h +++ b/lib/sfio/sfio.h @@ -136,7 +136,6 @@ extern "C" { #define SF_STATIC 0001000 /* a stream that cannot be freed */ #define SF_IOCHECK 0002000 /* call exceptf before doing IO */ #define SF_PUBLIC 0004000 /* SF_SHARE and follow physical seek */ -#define SF_MTSAFE 0010000 /* need thread safety */ #define SF_WHOLE 0020000 /* preserve wholeness of sfwrite/sfputr */ #define SF_FLAGS 0077177 /* PUBLIC FLAGS PASSABLE TO SFNEW() */ diff --git a/lib/sfio/sfopen.c b/lib/sfio/sfopen.c index 2b83fa030..d1dad6906 100644 --- a/lib/sfio/sfopen.c +++ b/lib/sfio/sfopen.c @@ -161,11 +161,9 @@ int _sftype(const char *mode, int *oflagsp, int *uflagp) sflags |= SF_READ | SF_WRITE; continue; case 'm': - sflags |= SF_MTSAFE; uflag = 0; continue; case 'u': - sflags &= (unsigned short)~SF_MTSAFE; uflag = 1; continue; default: -- 2.40.0