threads/141198: src/lib/libc/stdio does not properly initialize mutexes
Locked
threads/141198: src/lib/libc/stdio does not properly initialize mutexes
 
|
>Number: 141198 >Category: threads >Synopsis: src/lib/libc/stdio does not properly initialize mutexes >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-threads >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sat Dec 05 20:40:01 UTC 2009 >Closed-Date: >Last-Modified: >Originator: Jeremy Huddleston >Release: 8.0 >Organization: >Environment: NA >Description: libc/stdio assumes PTHREAD_MUTEX_INITIALIZER is NULL (which it is in FreeBSD), but this makes the code not as portable. Earlier versions of stdio did properly initialize the lock to PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the _extra extension substructure. >How-To-Repeat: >Fix: >Release-Note: >Audit-Trail: >Unformatted: _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
On Saturday 05 December 2009 3:34:41 pm Jeremy Huddleston wrote:
> > >Number: 141198 > >Category: threads > >Synopsis: src/lib/libc/stdio does not properly initialize mutexes > >Confidential: no > >Severity: non-critical > >Priority: low > >Responsible: freebsd-threads > >State: open > >Quarter: > >Keywords: > >Date-Required: > >Class: sw-bug > >Submitter-Id: current-users > >Arrival-Date: Sat Dec 05 20:40:01 UTC 2009 > >Closed-Date: > >Last-Modified: > >Originator: Jeremy Huddleston > >Release: 8.0 > >Organization: > Apple > >Environment: > NA > >Description: > libc/stdio assumes PTHREAD_MUTEX_INITIALIZER is NULL (which it is in > > Earlier versions of stdio did properly initialize the lock to PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the _extra extension substructure. This is my fault. I suspect it was more an error of omission on my part than assuming the default value of PTHREAD_MUTEX_INITIALIZER. :) Hmm, so I reviewed the change that removed INITEXTRA() and all the places it was used to construct a 'fake' FILE object it was passed to an internal stdio routine that did not use locking. One thing I do see that is an old bug is that the three static FILE structures used for stdin, stdout, and stderr have never had their internal locks properly initialized. Also, it seems __sfp() never initializes fp->_lock at all. Oh, it gets set to NULL via 'empty' in moreglue(). That is also an old bug. I think this will fix those problems: Index: stdio/findfp.c =================================================================== --- stdio/findfp.c (revision 199969) +++ stdio/findfp.c (working copy) @@ -61,6 +61,7 @@ ._read = __sread, \ ._seek = __sseek, \ ._write = __swrite, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ } /* the usual - (stdin + stdout + stderr) */ static FILE usual[FOPEN_MAX - 3]; @@ -96,7 +97,7 @@ int n; { struct glue *g; - static FILE empty; + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; FILE *p; g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); @@ -154,7 +155,7 @@ fp->_ub._size = 0; fp->_lb._base = NULL; /* no line buffer */ fp->_lb._size = 0; -/* fp->_lock = NULL; */ /* once set always set (reused) */ +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ fp->_orientation = 0; memset(&fp->_mbstate, 0, sizeof(mbstate_t)); return (fp); -- John Baldwin _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
In reply to this post by Jeremy Huddleston-6
The following reply was made to PR threads/141198; it has been noted by GNATS.
From: John Baldwin <[hidden email]> To: [hidden email] Cc: Jeremy Huddleston <[hidden email]>, [hidden email] Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes Date: Mon, 7 Dec 2009 08:50:37 -0500 On Saturday 05 December 2009 3:34:41 pm Jeremy Huddleston wrote: > > >Number: 141198 > >Category: threads > >Synopsis: src/lib/libc/stdio does not properly initialize mutexes > >Confidential: no > >Severity: non-critical > >Priority: low > >Responsible: freebsd-threads > >State: open > >Quarter: > >Keywords: > >Date-Required: > >Class: sw-bug > >Submitter-Id: current-users > >Arrival-Date: Sat Dec 05 20:40:01 UTC 2009 > >Closed-Date: > >Last-Modified: > >Originator: Jeremy Huddleston > >Release: 8.0 > >Organization: > Apple > >Environment: > NA > >Description: > libc/stdio assumes PTHREAD_MUTEX_INITIALIZER is NULL (which it is in FreeBSD), but this makes the code not as portable. > > Earlier versions of stdio did properly initialize the lock to PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the _extra extension substructure. This is my fault. I suspect it was more an error of omission on my part than assuming the default value of PTHREAD_MUTEX_INITIALIZER. :) Hmm, so I reviewed the change that removed INITEXTRA() and all the places it was used to construct a 'fake' FILE object it was passed to an internal stdio routine that did not use locking. One thing I do see that is an old bug is that the three static FILE structures used for stdin, stdout, and stderr have never had their internal locks properly initialized. Also, it seems __sfp() never initializes fp->_lock at all. Oh, it gets set to NULL via 'empty' in moreglue(). That is also an old bug. I think this will fix those problems: Index: stdio/findfp.c =================================================================== --- stdio/findfp.c (revision 199969) +++ stdio/findfp.c (working copy) @@ -61,6 +61,7 @@ ._read = __sread, \ ._seek = __sseek, \ ._write = __swrite, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ } /* the usual - (stdin + stdout + stderr) */ static FILE usual[FOPEN_MAX - 3]; @@ -96,7 +97,7 @@ int n; { struct glue *g; - static FILE empty; + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; FILE *p; g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); @@ -154,7 +155,7 @@ fp->_ub._size = 0; fp->_lb._base = NULL; /* no line buffer */ fp->_lb._size = 0; -/* fp->_lock = NULL; */ /* once set always set (reused) */ +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ fp->_orientation = 0; memset(&fp->_mbstate, 0, sizeof(mbstate_t)); return (fp); -- John Baldwin _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
In reply to this post by John Baldwin
On Monday 07 December 2009 8:50:37 am John Baldwin wrote:
> On Saturday 05 December 2009 3:34:41 pm Jeremy Huddleston wrote: > > > > >Number: 141198 > > >Category: threads > > >Synopsis: src/lib/libc/stdio does not properly initialize mutexes > > >Confidential: no > > >Severity: non-critical > > >Priority: low > > >Responsible: freebsd-threads > > >State: open > > >Quarter: > > >Keywords: > > >Date-Required: > > >Class: sw-bug > > >Submitter-Id: current-users > > >Arrival-Date: Sat Dec 05 20:40:01 UTC 2009 > > >Closed-Date: > > >Last-Modified: > > >Originator: Jeremy Huddleston > > >Release: 8.0 > > >Organization: > > Apple > > >Environment: > > NA > > >Description: > > libc/stdio assumes PTHREAD_MUTEX_INITIALIZER is NULL (which it is in > FreeBSD), but this makes the code not as portable. > > > > Earlier versions of stdio did properly initialize the lock to > PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the _extra > extension substructure. > > This is my fault. I suspect it was more an error of omission on my part than > assuming the default value of PTHREAD_MUTEX_INITIALIZER. :) Hmm, so I > reviewed the change that removed INITEXTRA() and all the places it was used to > construct a 'fake' FILE object it was passed to an internal stdio routine that > did not use locking. One thing I do see that is an old bug is that the three > static FILE structures used for stdin, stdout, and stderr have never had their > internal locks properly initialized. Also, it seems __sfp() never initializes > fp->_lock at all. Oh, it gets set to NULL via 'empty' in moreglue(). That is > also an old bug. I think this will fix those problems: > > Index: stdio/findfp.c > =================================================================== > --- stdio/findfp.c (revision 199969) > +++ stdio/findfp.c (working copy) > @@ -61,6 +61,7 @@ > ._read = __sread, \ > ._seek = __sseek, \ > ._write = __swrite, \ > + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ > } > /* the usual - (stdin + stdout + stderr) */ > static FILE usual[FOPEN_MAX - 3]; > @@ -96,7 +97,7 @@ > int n; > { > struct glue *g; > - static FILE empty; > + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; > FILE *p; > > g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); > @@ -154,7 +155,7 @@ > fp->_ub._size = 0; > fp->_lb._base = NULL; /* no line buffer */ > fp->_lb._size = 0; > -/* fp->_lock = NULL; */ /* once set always set (reused) */ > +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ > fp->_orientation = 0; > memset(&fp->_mbstate, 0, sizeof(mbstate_t)); > return (fp); Does this patch address the concerns? -- John Baldwin _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
I don't think that is sufficient.
On Jan 6, 2010, at 16:00, John Baldwin wrote: > On Monday 07 December 2009 8:50:37 am John Baldwin wrote: >> On Saturday 05 December 2009 3:34:41 pm Jeremy Huddleston wrote: >>> >>>> Number: 141198 >>>> Category: threads >>>> Synopsis: src/lib/libc/stdio does not properly initialize mutexes >>>> Confidential: no >>>> Severity: non-critical >>>> Priority: low >>>> Responsible: freebsd-threads >>>> State: open >>>> Quarter: >>>> Keywords: >>>> Date-Required: >>>> Class: sw-bug >>>> Submitter-Id: current-users >>>> Arrival-Date: Sat Dec 05 20:40:01 UTC 2009 >>>> Closed-Date: >>>> Last-Modified: >>>> Originator: Jeremy Huddleston >>>> Release: 8.0 >>>> Organization: >>> Apple >>>> Environment: >>> NA >>>> Description: >>> libc/stdio assumes PTHREAD_MUTEX_INITIALIZER is NULL (which it is in >> FreeBSD), but this makes the code not as portable. >>> >>> Earlier versions of stdio did properly initialize the lock to >> PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the _extra >> extension substructure. >> >> This is my fault. I suspect it was more an error of omission on my part than >> assuming the default value of PTHREAD_MUTEX_INITIALIZER. :) Hmm, so I >> reviewed the change that removed INITEXTRA() and all the places it was used to >> construct a 'fake' FILE object it was passed to an internal stdio routine that >> did not use locking. One thing I do see that is an old bug is that the three >> static FILE structures used for stdin, stdout, and stderr have never had their >> internal locks properly initialized. Also, it seems __sfp() never initializes >> fp->_lock at all. Oh, it gets set to NULL via 'empty' in moreglue(). That is >> also an old bug. I think this will fix those problems: >> >> Index: stdio/findfp.c >> =================================================================== >> --- stdio/findfp.c (revision 199969) >> +++ stdio/findfp.c (working copy) >> @@ -61,6 +61,7 @@ >> ._read = __sread, \ >> ._seek = __sseek, \ >> ._write = __swrite, \ >> + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ >> } >> /* the usual - (stdin + stdout + stderr) */ >> static FILE usual[FOPEN_MAX - 3]; >> @@ -96,7 +97,7 @@ >> int n; >> { >> struct glue *g; >> - static FILE empty; >> + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; >> FILE *p; >> >> g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); >> @@ -154,7 +155,7 @@ >> fp->_ub._size = 0; >> fp->_lb._base = NULL; /* no line buffer */ >> fp->_lb._size = 0; >> -/* fp->_lock = NULL; */ /* once set always set (reused) */ >> +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ >> fp->_orientation = 0; >> memset(&fp->_mbstate, 0, sizeof(mbstate_t)); >> return (fp); > > Does this patch address the concerns? vasprintf.c.patch:+ INITEXTRA(&f); vdprintf.c.patch:+ INITEXTRA(&f); vfprintf.c.patch:+ INITEXTRA(&fake); vfwprintf.c.patch:+ INITEXTRA(&fake); vsnprintf.c.patch:+ INITEXTRA(&f); vsprintf.c.patch:+ INITEXTRA(&f); vsscanf.c.patch:+ INITEXTRA(&f); vswprintf.c.patch:+ INITEXTRA(&f); vswscanf.c.patch:+ INITEXTRA(&f); |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
On Wednesday 06 January 2010 7:00:14 pm Jeremy Huddleston wrote:
> I don't think that is sufficient. > > On Jan 6, 2010, at 16:00, John Baldwin wrote: > > > On Monday 07 December 2009 8:50:37 am John Baldwin wrote: > >> On Saturday 05 December 2009 3:34:41 pm Jeremy Huddleston wrote: > >>> > >>>> Number: 141198 > >>>> Category: threads > >>>> Synopsis: src/lib/libc/stdio does not properly initialize mutexes > >>>> Confidential: no > >>>> Severity: non-critical > >>>> Priority: low > >>>> Responsible: freebsd-threads > >>>> State: open > >>>> Quarter: > >>>> Keywords: > >>>> Date-Required: > >>>> Class: sw-bug > >>>> Submitter-Id: current-users > >>>> Arrival-Date: Sat Dec 05 20:40:01 UTC 2009 > >>>> Closed-Date: > >>>> Last-Modified: > >>>> Originator: Jeremy Huddleston > >>>> Release: 8.0 > >>>> Organization: > >>> Apple > >>>> Environment: > >>> NA > >>>> Description: > >>> libc/stdio assumes PTHREAD_MUTEX_INITIALIZER is NULL (which it is in > >> FreeBSD), but this makes the code not as portable. > >>> > >>> Earlier versions of stdio did properly initialize the lock to > >> PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the _extra > >> extension substructure. > >> > >> This is my fault. I suspect it was more an error of omission on my part than > >> assuming the default value of PTHREAD_MUTEX_INITIALIZER. :) Hmm, so I > >> reviewed the change that removed INITEXTRA() and all the places it was used to > >> construct a 'fake' FILE object it was passed to an internal stdio routine that > >> did not use locking. One thing I do see that is an old bug is that the three > >> static FILE structures used for stdin, stdout, and stderr have never had their > >> internal locks properly initialized. Also, it seems __sfp() never initializes > >> fp->_lock at all. Oh, it gets set to NULL via 'empty' in moreglue(). That is > >> also an old bug. I think this will fix those problems: > >> > >> Index: stdio/findfp.c > >> =================================================================== > >> --- stdio/findfp.c (revision 199969) > >> +++ stdio/findfp.c (working copy) > >> @@ -61,6 +61,7 @@ > >> ._read = __sread, \ > >> ._seek = __sseek, \ > >> ._write = __swrite, \ > >> + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ > >> } > >> /* the usual - (stdin + stdout + stderr) */ > >> static FILE usual[FOPEN_MAX - 3]; > >> @@ -96,7 +97,7 @@ > >> int n; > >> { > >> struct glue *g; > >> - static FILE empty; > >> + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; > >> FILE *p; > >> > >> g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); > >> @@ -154,7 +155,7 @@ > >> fp->_ub._size = 0; > >> fp->_lb._base = NULL; /* no line buffer */ > >> fp->_lb._size = 0; > >> -/* fp->_lock = NULL; */ /* once set always set (reused) */ > >> +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ > >> fp->_orientation = 0; > >> memset(&fp->_mbstate, 0, sizeof(mbstate_t)); > >> return (fp); > > > > Does this patch address the concerns? > > I'm not sure if that is sufficient. I added it there and as part of INITEXTRA (which we reverted to in darwin) in the following. I can provide you with the full patches if you want, but there is a lot > > vasprintf.c.patch:+ INITEXTRA(&f); > vdprintf.c.patch:+ INITEXTRA(&f); > vfprintf.c.patch:+ INITEXTRA(&fake); > vfwprintf.c.patch:+ INITEXTRA(&fake); > vsnprintf.c.patch:+ INITEXTRA(&f); > vsprintf.c.patch:+ INITEXTRA(&f); > vsscanf.c.patch:+ INITEXTRA(&f); > vswprintf.c.patch:+ INITEXTRA(&f); > vswscanf.c.patch:+ INITEXTRA(&f); Ah, ok. In our stdio at least these are all dummy files that are passed to internal stdio routines that never do any locking (e.g. __vfprintf() which does no locking vs vfprintf() which does use the mutex locks). I'm not sure if that is also true for Darwin, but in theory it should be as these file objects are all private stack variables that no other threads can gain a reference to, so no locking is needed. -- John Baldwin _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
On Jan 7, 2010, at 09:56, John Baldwin wrote: >> vasprintf.c.patch:+ INITEXTRA(&f); >> vdprintf.c.patch:+ INITEXTRA(&f); >> vfprintf.c.patch:+ INITEXTRA(&fake); >> vfwprintf.c.patch:+ INITEXTRA(&fake); >> vsnprintf.c.patch:+ INITEXTRA(&f); >> vsprintf.c.patch:+ INITEXTRA(&f); >> vsscanf.c.patch:+ INITEXTRA(&f); >> vswprintf.c.patch:+ INITEXTRA(&f); >> vswscanf.c.patch:+ INITEXTRA(&f); > > Ah, ok. In our stdio at least these are all dummy files that are passed to > internal stdio routines that never do any locking (e.g. __vfprintf() which > does no locking vs vfprintf() which does use the mutex locks). I'm not sure > if that is also true for Darwin, but in theory it should be as these file > objects are all private stack variables that no other threads can gain a > reference to, so no locking is needed. If you're confident that won't be the case for FBSD, then I believe your initial patch is sufficient. |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
In reply to this post by Jeremy Huddleston-6
The following reply was made to PR threads/141198; it has been noted by GNATS.
From: Jeremy Huddleston <[hidden email]> To: John Baldwin <[hidden email]> Cc: [hidden email], [hidden email] Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes Date: Thu, 7 Jan 2010 10:44:54 -0500 --Apple-Mail-20--1043430417 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii On Jan 7, 2010, at 09:56, John Baldwin wrote: >> vasprintf.c.patch:+ INITEXTRA(&f); >> vdprintf.c.patch:+ INITEXTRA(&f); >> vfprintf.c.patch:+ INITEXTRA(&fake); >> vfwprintf.c.patch:+ INITEXTRA(&fake); >> vsnprintf.c.patch:+ INITEXTRA(&f); >> vsprintf.c.patch:+ INITEXTRA(&f); >> vsscanf.c.patch:+ INITEXTRA(&f); >> vswprintf.c.patch:+ INITEXTRA(&f); >> vswscanf.c.patch:+ INITEXTRA(&f); >=20 > Ah, ok. In our stdio at least these are all dummy files that are = passed to > internal stdio routines that never do any locking (e.g. __vfprintf() = which > does no locking vs vfprintf() which does use the mutex locks). I'm = not sure > if that is also true for Darwin, but in theory it should be as these = file > objects are all private stack variables that no other threads can gain = a > reference to, so no locking is needed. Yeah, we're just being cautious with these changes. It takes one clock = cycle and maintains the old (FBSD 7?) state of initializing the mutex = during INITEXTRA in those dumies... just in case something gets added = down the line which needs it. If you're confident that won't be the case for FBSD, then I believe your = initial patch is sufficient. --Apple-Mail-20--1043430417 Content-Disposition: attachment; filename=smime.p7s Content-Type: application/pkcs7-signature; name=smime.p7s Content-Transfer-Encoding: base64 MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIITbjCCAz8w ggKooAMCAQICAQ0wDQYJKoZIhvcNAQEFBQAwgdExCzAJBgNVBAYTAlpBMRUwEwYDVQQIEwxXZXN0 ZXJuIENhcGUxEjAQBgNVBAcTCUNhcGUgVG93bjEaMBgGA1UEChMRVGhhd3RlIENvbnN1bHRpbmcx KDAmBgNVBAsTH0NlcnRpZmljYXRpb24gU2VydmljZXMgRGl2aXNpb24xJDAiBgNVBAMTG1RoYXd0 ZSBQZXJzb25hbCBGcmVlbWFpbCBDQTErMCkGCSqGSIb3DQEJARYccGVyc29uYWwtZnJlZW1haWxA dGhhd3RlLmNvbTAeFw0wMzA3MTcwMDAwMDBaFw0xMzA3MTYyMzU5NTlaMGIxCzAJBgNVBAYTAlpB MSUwIwYDVQQKExxUaGF3dGUgQ29uc3VsdGluZyAoUHR5KSBMdGQuMSwwKgYDVQQDEyNUaGF3dGUg UGVyc29uYWwgRnJlZW1haWwgSXNzdWluZyBDQTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA xKY8VXNV+065yplaHmjAdQRwnd/p/6Me7L3N9VvyGna9fww6YfK/Uc4B1OVQCjDXAmNaLIkVcI7d yfArhVqqP3FWy688Cwfn8R+RNiQqE88r1fOCdz0Dviv+uxg+B79AgAJk16emu59l0cUqVIUPSAR/ p7bRPGEEQB5kGXJgt/sCAwEAAaOBlDCBkTASBgNVHRMBAf8ECDAGAQH/AgEAMEMGA1UdHwQ8MDow OKA2oDSGMmh0dHA6Ly9jcmwudGhhd3RlLmNvbS9UaGF3dGVQZXJzb25hbEZyZWVtYWlsQ0EuY3Js MAsGA1UdDwQEAwIBBjApBgNVHREEIjAgpB4wHDEaMBgGA1UEAxMRUHJpdmF0ZUxhYmVsMi0xMzgw DQYJKoZIhvcNAQEFBQADgYEASIzRUIPqCy7MDaNmrGcPf6+svsIXoUOWlJ1/TCG4+DYfqi2fNi/A 9BxQIJNwPP2t4WFiw9k6GX6EsZkbAMUaC4J0niVQlGLH2ydxVyWN3amcOY6MIE9lX5Xa9/eH1sYI Tq726jTlEBpbNU1341YheILcIRk13iSx0x1G/11fZU8wggM/MIICqKADAgECAgENMA0GCSqGSIb3 DQEBBQUAMIHRMQswCQYDVQQGEwJaQTEVMBMGA1UECBMMV2VzdGVybiBDYXBlMRIwEAYDVQQHEwlD YXBlIFRvd24xGjAYBgNVBAoTEVRoYXd0ZSBDb25zdWx0aW5nMSgwJgYDVQQLEx9DZXJ0aWZpY2F0 aW9uIFNlcnZpY2VzIERpdmlzaW9uMSQwIgYDVQQDExtUaGF3dGUgUGVyc29uYWwgRnJlZW1haWwg Q0ExKzApBgkqhkiG9w0BCQEWHHBlcnNvbmFsLWZyZWVtYWlsQHRoYXd0ZS5jb20wHhcNMDMwNzE3 MDAwMDAwWhcNMTMwNzE2MjM1OTU5WjBiMQswCQYDVQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENv bnN1bHRpbmcgKFB0eSkgTHRkLjEsMCoGA1UEAxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElz c3VpbmcgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMSmPFVzVftOucqZWh5owHUEcJ3f 6f+jHuy9zfVb8hp2vX8MOmHyv1HOAdTlUAow1wJjWiyJFXCO3cnwK4Vaqj9xVsuvPAsH5/EfkTYk KhPPK9Xzgnc9A74r/rsYPge/QIACZNenprufZdHFKlSFD0gEf6e20TxhBEAeZBlyYLf7AgMBAAGj gZQwgZEwEgYDVR0TAQH/BAgwBgEB/wIBADBDBgNVHR8EPDA6MDigNqA0hjJodHRwOi8vY3JsLnRo YXd0ZS5jb20vVGhhd3RlUGVyc29uYWxGcmVlbWFpbENBLmNybDALBgNVHQ8EBAMCAQYwKQYDVR0R BCIwIKQeMBwxGjAYBgNVBAMTEVByaXZhdGVMYWJlbDItMTM4MA0GCSqGSIb3DQEBBQUAA4GBAEiM 0VCD6gsuzA2jZqxnD3+vrL7CF6FDlpSdf0whuPg2H6otnzYvwPQcUCCTcDz9reFhYsPZOhl+hLGZ GwDFGguCdJ4lUJRix9sncVcljd2pnDmOjCBPZV+V2vf3h9bGCE6u9uo05RAaWzVNd+NWIXiC3CEZ Nd4ksdMdRv9dX2VPMIIGcDCCBdmgAwIBAgIQKF0Nr8sW2fhCBNsoUjwm8zANBgkqhkiG9w0BAQUF ADBiMQswCQYDVQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENvbnN1bHRpbmcgKFB0eSkgTHRkLjEs MCoGA1UEAxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElzc3VpbmcgQ0EwHhcNMDkwNTA0MDUy OTE0WhcNMTAwNTA0MDUyOTE0WjCCAnAxHzAdBgNVBAMTFlRoYXd0ZSBGcmVlbWFpbCBNZW1iZXIx JDAiBgkqhkiG9w0BCQEWFWplcmVteWh1QGJlcmtlbGV5LmVkdTErMCkGCSqGSIb3DQEJARYcamVy ZW15aHVAdWNsaW5rLmJlcmtlbGV5LmVkdTEsMCoGCSqGSIb3DQEJARYdamVyZW15aHVAdWNsaW5r NC5iZXJrZWxleS5lZHUxJzAlBgkqhkiG9w0BCQEWGGplcmVteWh1QGNzLmJlcmtlbGV5LmVkdTEp MCcGCSqGSIb3DQEJARYaamVyZW15QHVwZS5jcy5iZXJrZWxleS5lZHUxKTAnBgkqhkiG9w0BCQEW GmplcmVteWh1QGVlY3MuYmVya2VsZXkuZWR1MScwJQYJKoZIhvcNAQkBFhhqZXJlbXlodUBmcmVl ZGVza3RvcC5vcmcxJDAiBgkqhkiG9w0BCQEWFWplcmVteWh1QG1hY3BvcnRzLm9yZzElMCMGCSqG SIb3DQEJARYWamVyZW15QG91dGVyc3F1YXJlLm9yZzEgMB4GCSqGSIb3DQEJARYRamVyZW15aHVk QG1hYy5jb20xIzAhBgkqhkiG9w0BCQEWFGplcmVteUBodWRzY2FiaW4uY29tMSEwHwYJKoZIhvcN AQkBFhJqZXJlbXlodUBhcHBsZS5jb20xJTAjBgkqhkiG9w0BCQEWFmplcmVteUBvdXRlcnNxdWFy ZS5jb20xJTAjBgkqhkiG9w0BCQEWFnBheXBhbEBvdXRlcnNxdWFyZS5jb20xHzAdBgkqhkiG9w0B CQEWEGplcmVteWh1ZEBtZS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCwVnJ8 XrKgByWkhJhQDk7Kj45PnZYRXJNQfcTyBQsqSqfUh13Limf2qJTxpw8Mdq/SuNkO3ZjLkaYGPB4+ 8uaHdDqGEanq2wf4qKV4dyFEQO92mRQRxLijfBS4CunlSYzHuPd6g5osI0BVpFbNRswqOXWbHd1z XRVvRqpvYKQJFWLf3dqXU3zZO2nv4sabnovbNCKEO6HrxQeawFfwxL20adsK5F1ejK1VRSEsTzd7 BjNs8QTWC4qZKrrNuaPJLVt4LDbRXIqOggrZaOkggIBIIdXubjOrrpR41PvcvibfvYLUpo3bdX5e tWH/VU/ywIS3oIc4d+VtOL/O3YdCpX0FAgMBAAGjggGRMIIBjTCCAXsGA1UdEQSCAXIwggFugRVq ZXJlbXlodUBiZXJrZWxleS5lZHWBHGplcmVteWh1QHVjbGluay5iZXJrZWxleS5lZHWBHWplcmVt eWh1QHVjbGluazQuYmVya2VsZXkuZWR1gRhqZXJlbXlodUBjcy5iZXJrZWxleS5lZHWBGmplcmVt eUB1cGUuY3MuYmVya2VsZXkuZWR1gRpqZXJlbXlodUBlZWNzLmJlcmtlbGV5LmVkdYEYamVyZW15 aHVAZnJlZWRlc2t0b3Aub3JngRVqZXJlbXlodUBtYWNwb3J0cy5vcmeBFmplcmVteUBvdXRlcnNx dWFyZS5vcmeBEWplcmVteWh1ZEBtYWMuY29tgRRqZXJlbXlAaHVkc2NhYmluLmNvbYESamVyZW15 aHVAYXBwbGUuY29tgRZqZXJlbXlAb3V0ZXJzcXVhcmUuY29tgRZwYXlwYWxAb3V0ZXJzcXVhcmUu Y29tgRBqZXJlbXlodWRAbWUuY29tMAwGA1UdEwEB/wQCMAAwDQYJKoZIhvcNAQEFBQADgYEAMtx6 voXn2w2+kaevSb7REuy5TBAQNzwlcwLiaC44HMVhwQGEYG544mBabCqY2+MtLbEn2RDQGHArtuCA Tv9liObLp6UPNKo+8Bcd3edN0dlFSeb0wFPVt71e05dGeyIoBxIrM4ix2BON/SHcGsgt3n1DRXen JLYVV809vRtHQpowggZwMIIF2aADAgECAhBfIA3CIvCJAyf8rsNvgxtuMA0GCSqGSIb3DQEBBQUA MGIxCzAJBgNVBAYTAlpBMSUwIwYDVQQKExxUaGF3dGUgQ29uc3VsdGluZyAoUHR5KSBMdGQuMSww KgYDVQQDEyNUaGF3dGUgUGVyc29uYWwgRnJlZW1haWwgSXNzdWluZyBDQTAeFw0wOTA5MTQyMTM2 MjdaFw0xMDA5MTQyMTM2MjdaMIICcDEfMB0GA1UEAxMWVGhhd3RlIEZyZWVtYWlsIE1lbWJlcjEk MCIGCSqGSIb3DQEJARYVamVyZW15aHVAYmVya2VsZXkuZWR1MSswKQYJKoZIhvcNAQkBFhxqZXJl bXlodUB1Y2xpbmsuYmVya2VsZXkuZWR1MSwwKgYJKoZIhvcNAQkBFh1qZXJlbXlodUB1Y2xpbms0 LmJlcmtlbGV5LmVkdTEnMCUGCSqGSIb3DQEJARYYamVyZW15aHVAY3MuYmVya2VsZXkuZWR1MSkw JwYJKoZIhvcNAQkBFhpqZXJlbXlAdXBlLmNzLmJlcmtlbGV5LmVkdTEpMCcGCSqGSIb3DQEJARYa amVyZW15aHVAZWVjcy5iZXJrZWxleS5lZHUxJzAlBgkqhkiG9w0BCQEWGGplcmVteWh1QGZyZWVk ZXNrdG9wLm9yZzEkMCIGCSqGSIb3DQEJARYVamVyZW15aHVAbWFjcG9ydHMub3JnMSUwIwYJKoZI hvcNAQkBFhZqZXJlbXlAb3V0ZXJzcXVhcmUub3JnMSAwHgYJKoZIhvcNAQkBFhFqZXJlbXlodWRA bWFjLmNvbTEjMCEGCSqGSIb3DQEJARYUamVyZW15QGh1ZHNjYWJpbi5jb20xITAfBgkqhkiG9w0B CQEWEmplcmVteWh1QGFwcGxlLmNvbTElMCMGCSqGSIb3DQEJARYWamVyZW15QG91dGVyc3F1YXJl LmNvbTElMCMGCSqGSIb3DQEJARYWcGF5cGFsQG91dGVyc3F1YXJlLmNvbTEfMB0GCSqGSIb3DQEJ ARYQamVyZW15aHVkQG1lLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAL+c2RGH leO3G25PQEPEVsV3H/cWDewBCnMbqV0zgEg3hMyoRUG3aRUgH4gWbhVNkx/5t0A+mLQQWNnktg2J ku4MJJhHmarkxQAwITyamyO+37GHFl2d7oe5J7CFwg3Evf/2Lli0mfglfDHBy5YN9yURbSMVRaDV WGHhpYkqTwGXG2Bpai7oqdOlB0hDcRGE4Fv5aurxAuxyIohZMuxhZBzDfmidKsOUTnsz+NCUFIXK cMLYWwvH4XOBC4l0SU523phMyEW0OPas38EWd2NMCYaO1URA944+cS68DUvCqrrRzGmixY03PcaV uJ/+KA3L2u9esq8vt8s5m8aW8MWQWIkCAwEAAaOCAZEwggGNMIIBewYDVR0RBIIBcjCCAW6BFWpl cmVteWh1QGJlcmtlbGV5LmVkdYEcamVyZW15aHVAdWNsaW5rLmJlcmtlbGV5LmVkdYEdamVyZW15 aHVAdWNsaW5rNC5iZXJrZWxleS5lZHWBGGplcmVteWh1QGNzLmJlcmtlbGV5LmVkdYEaamVyZW15 QHVwZS5jcy5iZXJrZWxleS5lZHWBGmplcmVteWh1QGVlY3MuYmVya2VsZXkuZWR1gRhqZXJlbXlo dUBmcmVlZGVza3RvcC5vcmeBFWplcmVteWh1QG1hY3BvcnRzLm9yZ4EWamVyZW15QG91dGVyc3F1 YXJlLm9yZ4ERamVyZW15aHVkQG1hYy5jb22BFGplcmVteUBodWRzY2FiaW4uY29tgRJqZXJlbXlo dUBhcHBsZS5jb22BFmplcmVteUBvdXRlcnNxdWFyZS5jb22BFnBheXBhbEBvdXRlcnNxdWFyZS5j b22BEGplcmVteWh1ZEBtZS5jb20wDAYDVR0TAQH/BAIwADANBgkqhkiG9w0BAQUFAAOBgQBAga5a Jmkyd0TMiY0icyR7j5soyooiP4q9+Iu6lG+s/S+7vF5sDadCq+Y7US091MNT4LmbQehwwhi4jUWy EZ+KP9dhfWMqi51rZDbhWxAqAoKmgWgoQ9UsA4LqaC1wWlrM/DtzZ7+L5ZZ+MWlr94fDNL8qU3+y 3ZfiXgpWBV1x1zGCAxAwggMMAgEBMHYwYjELMAkGA1UEBhMCWkExJTAjBgNVBAoTHFRoYXd0ZSBD b25zdWx0aW5nIChQdHkpIEx0ZC4xLDAqBgNVBAMTI1RoYXd0ZSBQZXJzb25hbCBGcmVlbWFpbCBJ c3N1aW5nIENBAhBfIA3CIvCJAyf8rsNvgxtuMAkGBSsOAwIaBQCgggFvMBgGCSqGSIb3DQEJAzEL BgkqhkiG9w0BBwEwHAYJKoZIhvcNAQkFMQ8XDTEwMDEwNzE1NDQ1NVowIwYJKoZIhvcNAQkEMRYE FJ+UxYZBHHNMiSz6Pv24bfLGvnAJMIGFBgkrBgEEAYI3EAQxeDB2MGIxCzAJBgNVBAYTAlpBMSUw IwYDVQQKExxUaGF3dGUgQ29uc3VsdGluZyAoUHR5KSBMdGQuMSwwKgYDVQQDEyNUaGF3dGUgUGVy c29uYWwgRnJlZW1haWwgSXNzdWluZyBDQQIQKF0Nr8sW2fhCBNsoUjwm8zCBhwYLKoZIhvcNAQkQ AgsxeKB2MGIxCzAJBgNVBAYTAlpBMSUwIwYDVQQKExxUaGF3dGUgQ29uc3VsdGluZyAoUHR5KSBM dGQuMSwwKgYDVQQDEyNUaGF3dGUgUGVyc29uYWwgRnJlZW1haWwgSXNzdWluZyBDQQIQKF0Nr8sW 2fhCBNsoUjwm8zANBgkqhkiG9w0BAQEFAASCAQCGTG+uiPHmb1vEewjHH2qKqAEJ1NfjpV1Vos65 40HEWI7Uw/Md41MVn57f8Vg+TTGut/vihLimloWyvVjkK/r3e2VS/VWSiACTi//miJ0dUz1rrh/L kya+NuSqd37alkRjdgk7NdutZrpe//kxE03EMVapAcZXymeTpKL+4ZYPljoTOECOfzGK6mqBQEjs FjzsT081GuenlxHiRB6Dgh2y5Ogyy7/CeVeCrphN9Ww6LRZ85VBJLgkegpQEexJybTP9hbRdmItO 307qkzSWbGiB+jOawrCBUhJ16OhA69xem6VqmGF0lJ9FwNyGppLKMkY2RQlxTpkh/PCHyisYeWJM AAAAAAAA --Apple-Mail-20--1043430417-- _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
In reply to this post by Jeremy Huddleston-6
On Thursday 07 January 2010 10:44:54 am Jeremy Huddleston wrote:
> > On Jan 7, 2010, at 09:56, John Baldwin wrote: > > >> vasprintf.c.patch:+ INITEXTRA(&f); > >> vdprintf.c.patch:+ INITEXTRA(&f); > >> vfprintf.c.patch:+ INITEXTRA(&fake); > >> vfwprintf.c.patch:+ INITEXTRA(&fake); > >> vsnprintf.c.patch:+ INITEXTRA(&f); > >> vsprintf.c.patch:+ INITEXTRA(&f); > >> vsscanf.c.patch:+ INITEXTRA(&f); > >> vswprintf.c.patch:+ INITEXTRA(&f); > >> vswscanf.c.patch:+ INITEXTRA(&f); > > > > Ah, ok. In our stdio at least these are all dummy files that are passed to > > internal stdio routines that never do any locking (e.g. __vfprintf() which > > does no locking vs vfprintf() which does use the mutex locks). I'm not sure > > if that is also true for Darwin, but in theory it should be as these file > > objects are all private stack variables that no other threads can gain a > > reference to, so no locking is needed. > > Yeah, we're just being cautious with these changes. It takes one clock cycle > and maintains the old (FBSD 7?) state of initializing the mutex during > INITEXTRA in those dumies... just in case something gets added down the line > which needs it. > > If you're confident that won't be the case for FBSD, then I believe your > initial patch is sufficient. Ok. I actually think it would be nicer to provide some sort of macro to properly initialize the various 'fake' FILE objects. I will probably look at doing that in which case including an initializer for the lock should be simple. I actually don't like the amount of duplicated code to setup fake FILE objects as it is. Here is a larger patch which does this in addition to the earlier patch. Index: stdio/vsnprintf.c =================================================================== --- stdio/vsnprintf.c (revision 201516) +++ stdio/vsnprintf.c (working copy) @@ -47,7 +47,7 @@ size_t on; int ret; char dummy[2]; - FILE f; + FILE f = FAKE_FILE; on = n; if (n != 0) @@ -61,12 +61,9 @@ str = dummy; n = 1; } - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = n; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (on > 0) *f._p = '\0'; Index: stdio/vswprintf.c =================================================================== --- stdio/vswprintf.c (revision 201516) +++ stdio/vswprintf.c (working copy) @@ -45,7 +45,7 @@ { static const mbstate_t initial; mbstate_t mbs; - FILE f; + FILE f = FAKE_FILE; char *mbp; int ret, sverrno; size_t nwc; @@ -55,7 +55,6 @@ return (-1); } - f._file = -1; f._flags = __SWR | __SSTR | __SALC; f._bf._base = f._p = (unsigned char *)malloc(128); if (f._bf._base == NULL) { @@ -63,8 +62,6 @@ return (-1); } f._bf._size = f._w = 127; /* Leave room for the NUL */ - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfwprintf(&f, fmt, ap); if (ret < 0) { sverrno = errno; Index: stdio/vsscanf.c =================================================================== --- stdio/vsscanf.c (revision 201516) +++ stdio/vsscanf.c (working copy) @@ -55,16 +55,11 @@ vsscanf(const char * __restrict str, const char * __restrict fmt, __va_list ap) { - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SRD; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._r = strlen(str); f._read = eofread; - f._ub._base = NULL; - f._lb._base = NULL; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); return (__svfscanf(&f, fmt, ap)); } Index: stdio/snprintf.c =================================================================== --- stdio/snprintf.c (revision 201516) +++ stdio/snprintf.c (working copy) @@ -48,7 +48,7 @@ size_t on; int ret; va_list ap; - FILE f; + FILE f = FAKE_FILE; on = n; if (n != 0) @@ -56,12 +56,9 @@ if (n > INT_MAX) n = INT_MAX; va_start(ap, fmt); - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = n; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (on > 0) *f._p = '\0'; Index: stdio/xprintf.c =================================================================== --- stdio/xprintf.c (revision 201516) +++ stdio/xprintf.c (working copy) @@ -48,6 +48,7 @@ #include <wchar.h> #include "un-namespace.h" +#include "local.h" #include "printf.h" #include "fvwrite.h" @@ -575,7 +576,7 @@ __v3printf(FILE *fp, const char *fmt, int pct, va_list ap) { int ret; - FILE fake; + FILE fake = FAKE_FILE; unsigned char buf[BUFSIZ]; /* copy the important variables */ Index: stdio/vdprintf.c =================================================================== --- stdio/vdprintf.c (revision 201516) +++ stdio/vdprintf.c (working copy) @@ -39,7 +39,7 @@ int vdprintf(int fd, const char * __restrict fmt, va_list ap) { - FILE f; + FILE f = FAKE_FILE; unsigned char buf[BUFSIZ]; int ret; @@ -56,8 +56,6 @@ f._write = __swrite; f._bf._base = buf; f._bf._size = sizeof(buf); - f._orientation = 0; - bzero(&f._mbstate, sizeof(f._mbstate)); if ((ret = __vfprintf(&f, fmt, ap)) < 0) return (ret); Index: stdio/vfprintf.c =================================================================== --- stdio/vfprintf.c (revision 201516) +++ stdio/vfprintf.c (working copy) @@ -169,7 +169,7 @@ __sbprintf(FILE *fp, const char *fmt, va_list ap) { int ret; - FILE fake; + FILE fake = FAKE_FILE; unsigned char buf[BUFSIZ]; /* XXX This is probably not needed. */ Index: stdio/local.h =================================================================== --- stdio/local.h (revision 201516) +++ stdio/local.h (working copy) @@ -110,6 +110,14 @@ } /* + * Structure initializations for 'fake' FILE objects. + */ +#define FAKE_FILE { \ + ._file = -1, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ +} + +/* * Set the orientation for a stream. If o > 0, the stream has wide- * orientation. If o < 0, the stream has byte-orientation. */ Index: stdio/vsprintf.c =================================================================== --- stdio/vsprintf.c (revision 201516) +++ stdio/vsprintf.c (working copy) @@ -44,14 +44,11 @@ vsprintf(char * __restrict str, const char * __restrict fmt, __va_list ap) { int ret; - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = INT_MAX; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); *f._p = 0; return (ret); Index: stdio/findfp.c =================================================================== --- stdio/findfp.c (revision 201516) +++ stdio/findfp.c (working copy) @@ -61,6 +61,7 @@ ._read = __sread, \ ._seek = __sseek, \ ._write = __swrite, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ } /* the usual - (stdin + stdout + stderr) */ static FILE usual[FOPEN_MAX - 3]; @@ -96,7 +97,7 @@ int n; { struct glue *g; - static FILE empty; + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; FILE *p; g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); @@ -154,7 +155,7 @@ fp->_ub._size = 0; fp->_lb._base = NULL; /* no line buffer */ fp->_lb._size = 0; -/* fp->_lock = NULL; */ /* once set always set (reused) */ +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ fp->_orientation = 0; memset(&fp->_mbstate, 0, sizeof(mbstate_t)); return (fp); Index: stdio/vasprintf.c =================================================================== --- stdio/vasprintf.c (revision 201516) +++ stdio/vasprintf.c (working copy) @@ -42,9 +42,8 @@ __va_list ap; { int ret; - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SWR | __SSTR | __SALC; f._bf._base = f._p = (unsigned char *)malloc(128); if (f._bf._base == NULL) { @@ -53,8 +52,6 @@ return (-1); } f._bf._size = f._w = 127; /* Leave room for the NUL */ - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (ret < 0) { free(f._bf._base); Index: stdio/vswscanf.c =================================================================== --- stdio/vswscanf.c (revision 201516) +++ stdio/vswscanf.c (working copy) @@ -62,7 +62,7 @@ { static const mbstate_t initial; mbstate_t mbs; - FILE f; + FILE f = FAKE_FILE; char *mbstr; size_t mlen; int r; @@ -80,15 +80,10 @@ free(mbstr); return (EOF); } - f._file = -1; f._flags = __SRD; f._bf._base = f._p = (unsigned char *)mbstr; f._bf._size = f._r = mlen; f._read = eofread; - f._ub._base = NULL; - f._lb._base = NULL; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); r = __vfwscanf(&f, fmt, ap); free(mbstr); -- John Baldwin _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
|
|
In reply to this post by Jeremy Huddleston-6
The following reply was made to PR threads/141198; it has been noted by GNATS.
From: John Baldwin <[hidden email]> To: Jeremy Huddleston <[hidden email]> Cc: [hidden email], [hidden email] Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes Date: Thu, 7 Jan 2010 13:15:25 -0500 On Thursday 07 January 2010 10:44:54 am Jeremy Huddleston wrote: > > On Jan 7, 2010, at 09:56, John Baldwin wrote: > > >> vasprintf.c.patch:+ INITEXTRA(&f); > >> vdprintf.c.patch:+ INITEXTRA(&f); > >> vfprintf.c.patch:+ INITEXTRA(&fake); > >> vfwprintf.c.patch:+ INITEXTRA(&fake); > >> vsnprintf.c.patch:+ INITEXTRA(&f); > >> vsprintf.c.patch:+ INITEXTRA(&f); > >> vsscanf.c.patch:+ INITEXTRA(&f); > >> vswprintf.c.patch:+ INITEXTRA(&f); > >> vswscanf.c.patch:+ INITEXTRA(&f); > > > > Ah, ok. In our stdio at least these are all dummy files that are passed to > > internal stdio routines that never do any locking (e.g. __vfprintf() which > > does no locking vs vfprintf() which does use the mutex locks). I'm not sure > > if that is also true for Darwin, but in theory it should be as these file > > objects are all private stack variables that no other threads can gain a > > reference to, so no locking is needed. > > Yeah, we're just being cautious with these changes. It takes one clock cycle > and maintains the old (FBSD 7?) state of initializing the mutex during > INITEXTRA in those dumies... just in case something gets added down the line > which needs it. > > If you're confident that won't be the case for FBSD, then I believe your > initial patch is sufficient. Ok. I actually think it would be nicer to provide some sort of macro to properly initialize the various 'fake' FILE objects. I will probably look at doing that in which case including an initializer for the lock should be simple. I actually don't like the amount of duplicated code to setup fake FILE objects as it is. Here is a larger patch which does this in addition to the earlier patch. Index: stdio/vsnprintf.c =================================================================== --- stdio/vsnprintf.c (revision 201516) +++ stdio/vsnprintf.c (working copy) @@ -47,7 +47,7 @@ size_t on; int ret; char dummy[2]; - FILE f; + FILE f = FAKE_FILE; on = n; if (n != 0) @@ -61,12 +61,9 @@ str = dummy; n = 1; } - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = n; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (on > 0) *f._p = '\0'; Index: stdio/vswprintf.c =================================================================== --- stdio/vswprintf.c (revision 201516) +++ stdio/vswprintf.c (working copy) @@ -45,7 +45,7 @@ { static const mbstate_t initial; mbstate_t mbs; - FILE f; + FILE f = FAKE_FILE; char *mbp; int ret, sverrno; size_t nwc; @@ -55,7 +55,6 @@ return (-1); } - f._file = -1; f._flags = __SWR | __SSTR | __SALC; f._bf._base = f._p = (unsigned char *)malloc(128); if (f._bf._base == NULL) { @@ -63,8 +62,6 @@ return (-1); } f._bf._size = f._w = 127; /* Leave room for the NUL */ - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfwprintf(&f, fmt, ap); if (ret < 0) { sverrno = errno; Index: stdio/vsscanf.c =================================================================== --- stdio/vsscanf.c (revision 201516) +++ stdio/vsscanf.c (working copy) @@ -55,16 +55,11 @@ vsscanf(const char * __restrict str, const char * __restrict fmt, __va_list ap) { - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SRD; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._r = strlen(str); f._read = eofread; - f._ub._base = NULL; - f._lb._base = NULL; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); return (__svfscanf(&f, fmt, ap)); } Index: stdio/snprintf.c =================================================================== --- stdio/snprintf.c (revision 201516) +++ stdio/snprintf.c (working copy) @@ -48,7 +48,7 @@ size_t on; int ret; va_list ap; - FILE f; + FILE f = FAKE_FILE; on = n; if (n != 0) @@ -56,12 +56,9 @@ if (n > INT_MAX) n = INT_MAX; va_start(ap, fmt); - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = n; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (on > 0) *f._p = '\0'; Index: stdio/xprintf.c =================================================================== --- stdio/xprintf.c (revision 201516) +++ stdio/xprintf.c (working copy) @@ -48,6 +48,7 @@ #include <wchar.h> #include "un-namespace.h" +#include "local.h" #include "printf.h" #include "fvwrite.h" @@ -575,7 +576,7 @@ __v3printf(FILE *fp, const char *fmt, int pct, va_list ap) { int ret; - FILE fake; + FILE fake = FAKE_FILE; unsigned char buf[BUFSIZ]; /* copy the important variables */ Index: stdio/vdprintf.c =================================================================== --- stdio/vdprintf.c (revision 201516) +++ stdio/vdprintf.c (working copy) @@ -39,7 +39,7 @@ int vdprintf(int fd, const char * __restrict fmt, va_list ap) { - FILE f; + FILE f = FAKE_FILE; unsigned char buf[BUFSIZ]; int ret; @@ -56,8 +56,6 @@ f._write = __swrite; f._bf._base = buf; f._bf._size = sizeof(buf); - f._orientation = 0; - bzero(&f._mbstate, sizeof(f._mbstate)); if ((ret = __vfprintf(&f, fmt, ap)) < 0) return (ret); Index: stdio/vfprintf.c =================================================================== --- stdio/vfprintf.c (revision 201516) +++ stdio/vfprintf.c (working copy) @@ -169,7 +169,7 @@ __sbprintf(FILE *fp, const char *fmt, va_list ap) { int ret; - FILE fake; + FILE fake = FAKE_FILE; unsigned char buf[BUFSIZ]; /* XXX This is probably not needed. */ Index: stdio/local.h =================================================================== --- stdio/local.h (revision 201516) +++ stdio/local.h (working copy) @@ -110,6 +110,14 @@ } /* + * Structure initializations for 'fake' FILE objects. + */ +#define FAKE_FILE { \ + ._file = -1, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ +} + +/* * Set the orientation for a stream. If o > 0, the stream has wide- * orientation. If o < 0, the stream has byte-orientation. */ Index: stdio/vsprintf.c =================================================================== --- stdio/vsprintf.c (revision 201516) +++ stdio/vsprintf.c (working copy) @@ -44,14 +44,11 @@ vsprintf(char * __restrict str, const char * __restrict fmt, __va_list ap) { int ret; - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SWR | __SSTR; f._bf._base = f._p = (unsigned char *)str; f._bf._size = f._w = INT_MAX; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); *f._p = 0; return (ret); Index: stdio/findfp.c =================================================================== --- stdio/findfp.c (revision 201516) +++ stdio/findfp.c (working copy) @@ -61,6 +61,7 @@ ._read = __sread, \ ._seek = __sseek, \ ._write = __swrite, \ + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ } /* the usual - (stdin + stdout + stderr) */ static FILE usual[FOPEN_MAX - 3]; @@ -96,7 +97,7 @@ int n; { struct glue *g; - static FILE empty; + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; FILE *p; g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); @@ -154,7 +155,7 @@ fp->_ub._size = 0; fp->_lb._base = NULL; /* no line buffer */ fp->_lb._size = 0; -/* fp->_lock = NULL; */ /* once set always set (reused) */ +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ fp->_orientation = 0; memset(&fp->_mbstate, 0, sizeof(mbstate_t)); return (fp); Index: stdio/vasprintf.c =================================================================== --- stdio/vasprintf.c (revision 201516) +++ stdio/vasprintf.c (working copy) @@ -42,9 +42,8 @@ __va_list ap; { int ret; - FILE f; + FILE f = FAKE_FILE; - f._file = -1; f._flags = __SWR | __SSTR | __SALC; f._bf._base = f._p = (unsigned char *)malloc(128); if (f._bf._base == NULL) { @@ -53,8 +52,6 @@ return (-1); } f._bf._size = f._w = 127; /* Leave room for the NUL */ - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); ret = __vfprintf(&f, fmt, ap); if (ret < 0) { free(f._bf._base); Index: stdio/vswscanf.c =================================================================== --- stdio/vswscanf.c (revision 201516) +++ stdio/vswscanf.c (working copy) @@ -62,7 +62,7 @@ { static const mbstate_t initial; mbstate_t mbs; - FILE f; + FILE f = FAKE_FILE; char *mbstr; size_t mlen; int r; @@ -80,15 +80,10 @@ free(mbstr); return (EOF); } - f._file = -1; f._flags = __SRD; f._bf._base = f._p = (unsigned char *)mbstr; f._bf._size = f._r = mlen; f._read = eofread; - f._ub._base = NULL; - f._lb._base = NULL; - f._orientation = 0; - memset(&f._mbstate, 0, sizeof(mbstate_t)); r = __vfwscanf(&f, fmt, ap); free(mbstr); -- John Baldwin _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-threads To unsubscribe, send any mail to "[hidden email]" |
«
Return to freebsd-threads
|
1 view|%1 views
Loading...
| Powered by Nabble | Edit this page |