Re: Goodbye to you my file descriptor - take 2
Fixed, thanks. On Wed, Nov 21, 2012 at 06:33:46PM +0100, rustyBSD wrote: > Hi, > I know I'm repeating, but I make a codescanneron my free > timeand sometimes I run it on /usr/src. > > So, here is a patch to fix a file descriptor leak. > > (I know, tabs are gone, how to avoid that on thunderbird?) > > > --- src/usr.bin/cu/xmodem.c2012-07-11 08:39:32.0 +0200 > +++ src/usr.bin/cu/xmodem.c2012-11-21 13:20:21.782313183 +0100 > @@ -172,6 +172,6 @@ > set_termios(); > > sigaction(SIGINT, &oact, NULL); > - > +fclose(f); > return; > }
Re: Goodbye to you my file descriptor - take 2
On 2012/11/21 18:33, rustyBSD wrote: > Hi, > I know I'm repeating, but I make a codescanneron my free > timeand sometimes I run it on /usr/src. > > So, here is a patch to fix a file descriptor leak. > > (I know, tabs are gone, how to avoid that on thunderbird?) see the git-format-patch(1) manpage, this has details of how to unbreak a couple of common MTAs. (including -p in the diff flags is often useful too, btw).
Goodbye to you my file descriptor - take 2
Hi, I know I'm repeating, but I make a codescanneron my free timeand sometimes I run it on /usr/src. So, here is a patch to fix a file descriptor leak. (I know, tabs are gone, how to avoid that on thunderbird?) --- src/usr.bin/cu/xmodem.c2012-07-11 08:39:32.0 +0200 +++ src/usr.bin/cu/xmodem.c2012-11-21 13:20:21.782313183 +0100 @@ -172,6 +172,6 @@ set_termios(); sigaction(SIGINT, &oact, NULL); - +fclose(f); return; }
Re: Goodbye to you my file descriptor
Thanks for fixing my mistake :-)
On Sat, Nov 3, 2012 at 6:57 PM, Christiano F. Haesbaert
wrote:
> On Tue, Oct 30, 2012 at 04:44:36PM +0100, rustyBSD wrote:
>> Le 30/10/2012 15:32, Christiano F. Haesbaert a ?crit :
>> > That should be an access(2) call.
>> Yes.Something like this - also moved len to size_t,
>> as strlen() is size_t:
>>
>>
>> --- dired.cWed Mar 14 14:56:35 2012
>> +++ dired.cTue Oct 30 16:23:00 2012
>> @@ -724,9 +724,10 @@
>> dired_(char *dname)
>> {
>> struct buffer*bp;
>> -int len, i;
>> +int i;
>> +size_t len;
>>
>> -if ((fopen(dname,"r")) == NULL) {
>> +if (access(dname, R_OK) == -1) {
>> if (errno == EACCES)
>> ewprintf("Permission denied");
>> return (NULL);
>>
>
> Your diff got mangled, it replaced tabs for spaces, anyway, I think we
> should also check for X_OK, otherwise we can't list the directory
> anyway.
>
> We still get the message "File is not readable", but I believe it's more
> useful than showing an empty directory.
>
> Index: dired.c
> ===
> RCS file: /cvs/src/usr.bin/mg/dired.c,v
> retrieving revision 1.51
> diff -d -u -p -r1.51 dired.c
> --- dired.c 14 Mar 2012 13:56:35 - 1.51
> +++ dired.c 3 Nov 2012 14:47:18 -
> @@ -724,9 +724,10 @@ struct buffer *
> dired_(char *dname)
> {
> struct buffer *bp;
> - int len, i;
> + int i;
> + size_t len;
>
> - if ((fopen(dname,"r")) == NULL) {
> + if ((access(dname, R_OK | X_OK)) == -1) {
> if (errno == EACCES)
> ewprintf("Permission denied");
> return (NULL);
>
--
Brightest day,
Blackest night,
No bug shall escape my sight,
And those who worship evil's mind,
be wary of my powers,
puffy lantern's light !
Re: Goodbye to you my file descriptor
On Tue, Oct 30, 2012 at 04:44:36PM +0100, rustyBSD wrote:
> Le 30/10/2012 15:32, Christiano F. Haesbaert a ?crit :
> > That should be an access(2) call.
> Yes.Something like this - also moved len to size_t,
> as strlen() is size_t:
>
>
> --- dired.cWed Mar 14 14:56:35 2012
> +++ dired.cTue Oct 30 16:23:00 2012
> @@ -724,9 +724,10 @@
> dired_(char *dname)
> {
> struct buffer*bp;
> -int len, i;
> +int i;
> +size_t len;
>
> -if ((fopen(dname,"r")) == NULL) {
> +if (access(dname, R_OK) == -1) {
> if (errno == EACCES)
> ewprintf("Permission denied");
> return (NULL);
>
Your diff got mangled, it replaced tabs for spaces, anyway, I think we
should also check for X_OK, otherwise we can't list the directory
anyway.
We still get the message "File is not readable", but I believe it's more
useful than showing an empty directory.
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.51
diff -d -u -p -r1.51 dired.c
--- dired.c 14 Mar 2012 13:56:35 - 1.51
+++ dired.c 3 Nov 2012 14:47:18 -
@@ -724,9 +724,10 @@ struct buffer *
dired_(char *dname)
{
struct buffer *bp;
- int len, i;
+ int i;
+ size_t len;
- if ((fopen(dname,"r")) == NULL) {
+ if ((access(dname, R_OK | X_OK)) == -1) {
if (errno == EACCES)
ewprintf("Permission denied");
return (NULL);
Re: Goodbye to you my file descriptor
Le 30/10/2012 15:32, Christiano F. Haesbaert a écrit :
> That should be an access(2) call.
Yes.Something like this - also moved len to size_t,
as strlen() is size_t:
--- dired.cWed Mar 14 14:56:35 2012
+++ dired.cTue Oct 30 16:23:00 2012
@@ -724,9 +724,10 @@
dired_(char *dname)
{
struct buffer*bp;
-int len, i;
+int i;
+size_t len;
-if ((fopen(dname,"r")) == NULL) {
+if (access(dname, R_OK) == -1) {
if (errno == EACCES)
ewprintf("Permission denied");
return (NULL);
Re: Goodbye to you my file descriptor
On Tue, Oct 30, 2012 at 11:57:05AM -0400, Okan Demirmen wrote: > On Tue, Oct 30, 2012 at 11:53 AM, Christiano F. Haesbaert > wrote: > > On 30 October 2012 16:52, Christiano F. Haesbaert > > wrote: > >> On 30 October 2012 16:45, Okan Demirmen wrote: > >>> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert > >>> wrote: > On 30 October 2012 15:03, Christiano F. Haesbaert > wrote: > That should be an access(2) call. > > >>> > >>> or stat(2) due to tctu. > >> > >> I believe in that case it would be the same, since there is still a > >> window between stat(2)/access(2) and open(2). > > > > I mean, considering he would open/stat/close and open again. > > I didn't actually look at the code; I just noticed the words > permission and access(2) and hit reply :) > Perhaps you meant fstat? Looking at the code, it doesn't look like there's any way to fix the TOCTOU issue without resorting to a complete overhaul, and instead using the openat() family of calls. OTOH, it looks like the permission check is just for sanity--early failure.
Re: Goodbye to you my file descriptor
On 30 October 2012 16:57, Okan Demirmen wrote:
> On Tue, Oct 30, 2012 at 11:53 AM, Christiano F. Haesbaert
> wrote:
>> On 30 October 2012 16:52, Christiano F. Haesbaert
>> wrote:
>>> On 30 October 2012 16:45, Okan Demirmen wrote:
On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
wrote:
> On 30 October 2012 15:03, Christiano F. Haesbaert
> wrote:
>> On 30 October 2012 15:00, Mike Belopuhov wrote:
>>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>>> wrote:
On 30 October 2012 14:36, rustyBSD wrote:
> MMmhh...
>
> == /usr/src/usr.bin/mg/dired.c ==
> Go look the line 729:
>
> if ((fopen(dname,"r")) == NULL) {
> ...
>
> Now you can cry
>
What is your point ?
>>>
>>> you leak a FILE object and a descriptor.
>>
>> Aww jesus, completely missed it !
>
> So that was just to check permission:
>
> ==
> http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/mg/dired.c.diff?r1=1.45;r2=1.46
>
> From the Loganaden Velvindron:
> Make dired more sane (and emacslike):
> * Position cursor at first filename after ..
> * Don't reposition cursor on reopening
> * Check for permission before attempting to open directory
>
> I took forever to get this in. Thanks, Logan for being patient!
> ==
>
> That should be an access(2) call.
>
or stat(2) due to tctu.
>>>
>>> I believe in that case it would be the same, since there is still a
>>> window between stat(2)/access(2) and open(2).
>>
>> I mean, considering he would open/stat/close and open again.
>
> I didn't actually look at the code; I just noticed the words
> permission and access(2) and hit reply :)
>
> Cheers.
Today is definately not my day, forgot that stat takes a path and not an fd.
Re: Goodbye to you my file descriptor
On Tue, Oct 30, 2012 at 11:53 AM, Christiano F. Haesbaert
wrote:
> On 30 October 2012 16:52, Christiano F. Haesbaert
> wrote:
>> On 30 October 2012 16:45, Okan Demirmen wrote:
>>> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
>>> wrote:
On 30 October 2012 15:03, Christiano F. Haesbaert
wrote:
> On 30 October 2012 15:00, Mike Belopuhov wrote:
>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>> wrote:
>>> On 30 October 2012 14:36, rustyBSD wrote:
MMmhh...
== /usr/src/usr.bin/mg/dired.c ==
Go look the line 729:
if ((fopen(dname,"r")) == NULL) {
...
Now you can cry
>>>
>>> What is your point ?
>>>
>>
>> you leak a FILE object and a descriptor.
>
> Aww jesus, completely missed it !
So that was just to check permission:
==
http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/mg/dired.c.diff?r1=1.45;r2=1.46
From the Loganaden Velvindron:
Make dired more sane (and emacslike):
* Position cursor at first filename after ..
* Don't reposition cursor on reopening
* Check for permission before attempting to open directory
I took forever to get this in. Thanks, Logan for being patient!
==
That should be an access(2) call.
>>>
>>> or stat(2) due to tctu.
>>
>> I believe in that case it would be the same, since there is still a
>> window between stat(2)/access(2) and open(2).
>
> I mean, considering he would open/stat/close and open again.
I didn't actually look at the code; I just noticed the words
permission and access(2) and hit reply :)
Cheers.
Re: Goodbye to you my file descriptor
On 30 October 2012 16:52, Christiano F. Haesbaert
wrote:
> On 30 October 2012 16:45, Okan Demirmen wrote:
>> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
>> wrote:
>>> On 30 October 2012 15:03, Christiano F. Haesbaert
>>> wrote:
On 30 October 2012 15:00, Mike Belopuhov wrote:
> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
> wrote:
>> On 30 October 2012 14:36, rustyBSD wrote:
>>> MMmhh...
>>>
>>> == /usr/src/usr.bin/mg/dired.c ==
>>> Go look the line 729:
>>>
>>> if ((fopen(dname,"r")) == NULL) {
>>> ...
>>>
>>> Now you can cry
>>>
>>
>> What is your point ?
>>
>
> you leak a FILE object and a descriptor.
Aww jesus, completely missed it !
>>>
>>> So that was just to check permission:
>>>
>>> ==
>>> http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/mg/dired.c.diff?r1=1.45;r2=1.46
>>>
>>> From the Loganaden Velvindron:
>>> Make dired more sane (and emacslike):
>>> * Position cursor at first filename after ..
>>> * Don't reposition cursor on reopening
>>> * Check for permission before attempting to open directory
>>>
>>> I took forever to get this in. Thanks, Logan for being patient!
>>> ==
>>>
>>> That should be an access(2) call.
>>>
>>
>> or stat(2) due to tctu.
>
> I believe in that case it would be the same, since there is still a
> window between stat(2)/access(2) and open(2).
I mean, considering he would open/stat/close and open again.
Re: Goodbye to you my file descriptor
On 30 October 2012 16:45, Okan Demirmen wrote:
> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
> wrote:
>> On 30 October 2012 15:03, Christiano F. Haesbaert
>> wrote:
>>> On 30 October 2012 15:00, Mike Belopuhov wrote:
On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
wrote:
> On 30 October 2012 14:36, rustyBSD wrote:
>> MMmhh...
>>
>> == /usr/src/usr.bin/mg/dired.c ==
>> Go look the line 729:
>>
>> if ((fopen(dname,"r")) == NULL) {
>> ...
>>
>> Now you can cry
>>
>
> What is your point ?
>
you leak a FILE object and a descriptor.
>>>
>>> Aww jesus, completely missed it !
>>
>> So that was just to check permission:
>>
>> ==
>> http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/mg/dired.c.diff?r1=1.45;r2=1.46
>>
>> From the Loganaden Velvindron:
>> Make dired more sane (and emacslike):
>> * Position cursor at first filename after ..
>> * Don't reposition cursor on reopening
>> * Check for permission before attempting to open directory
>>
>> I took forever to get this in. Thanks, Logan for being patient!
>> ==
>>
>> That should be an access(2) call.
>>
>
> or stat(2) due to tctu.
I believe in that case it would be the same, since there is still a
window between stat(2)/access(2) and open(2).
Re: Goodbye to you my file descriptor
On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
wrote:
> On 30 October 2012 15:03, Christiano F. Haesbaert
> wrote:
>> On 30 October 2012 15:00, Mike Belopuhov wrote:
>>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>>> wrote:
On 30 October 2012 14:36, rustyBSD wrote:
> MMmhh...
>
> == /usr/src/usr.bin/mg/dired.c ==
> Go look the line 729:
>
> if ((fopen(dname,"r")) == NULL) {
> ...
>
> Now you can cry
>
What is your point ?
>>>
>>> you leak a FILE object and a descriptor.
>>
>> Aww jesus, completely missed it !
>
> So that was just to check permission:
>
> ==
> http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/mg/dired.c.diff?r1=1.45;r2=1.46
>
> From the Loganaden Velvindron:
> Make dired more sane (and emacslike):
> * Position cursor at first filename after ..
> * Don't reposition cursor on reopening
> * Check for permission before attempting to open directory
>
> I took forever to get this in. Thanks, Logan for being patient!
> ==
>
> That should be an access(2) call.
>
or stat(2) due to tctu.
Re: Goodbye to you my file descriptor
On 30 October 2012 15:03, Christiano F. Haesbaert
wrote:
> On 30 October 2012 15:00, Mike Belopuhov wrote:
>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>> wrote:
>>> On 30 October 2012 14:36, rustyBSD wrote:
MMmhh...
== /usr/src/usr.bin/mg/dired.c ==
Go look the line 729:
if ((fopen(dname,"r")) == NULL) {
...
Now you can cry
>>>
>>> What is your point ?
>>>
>>
>> you leak a FILE object and a descriptor.
>
> Aww jesus, completely missed it !
So that was just to check permission:
==
http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/mg/dired.c.diff?r1=1.45;r2=1.46
>From the Loganaden Velvindron:
Make dired more sane (and emacslike):
* Position cursor at first filename after ..
* Don't reposition cursor on reopening
* Check for permission before attempting to open directory
I took forever to get this in. Thanks, Logan for being patient!
==
That should be an access(2) call.
Re: Goodbye to you my file descriptor
On 30 October 2012 15:00, Mike Belopuhov wrote:
> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
> wrote:
>> On 30 October 2012 14:36, rustyBSD wrote:
>>> MMmhh...
>>>
>>> == /usr/src/usr.bin/mg/dired.c ==
>>> Go look the line 729:
>>>
>>> if ((fopen(dname,"r")) == NULL) {
>>> ...
>>>
>>> Now you can cry
>>>
>>
>> What is your point ?
>>
>
> you leak a FILE object and a descriptor.
Aww jesus, completely missed it !
Re: Goodbye to you my file descriptor
On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
wrote:
> On 30 October 2012 14:36, rustyBSD wrote:
>> MMmhh...
>>
>> == /usr/src/usr.bin/mg/dired.c ==
>> Go look the line 729:
>>
>> if ((fopen(dname,"r")) == NULL) {
>> ...
>>
>> Now you can cry
>>
>
> What is your point ?
>
you leak a FILE object and a descriptor.
Re: Goodbye to you my file descriptor
On 30 October 2012 14:58, Christiano F. Haesbaert
wrote:
> On 30 October 2012 14:36, rustyBSD wrote:
>> MMmhh...
>>
>> == /usr/src/usr.bin/mg/dired.c ==
>> Go look the line 729:
>>
>> if ((fopen(dname,"r")) == NULL) {
>> ...
>>
>> Now you can cry
>>
>
> What is your point ?
Hmm just noticed there are some calls that do not check the NULL case,
looks like you have a diff to write :).
Re: Goodbye to you my file descriptor
On 30 October 2012 14:36, rustyBSD wrote:
> MMmhh...
>
> == /usr/src/usr.bin/mg/dired.c ==
> Go look the line 729:
>
> if ((fopen(dname,"r")) == NULL) {
> ...
>
> Now you can cry
>
What is your point ?
Goodbye to you my file descriptor
MMmhh...
== /usr/src/usr.bin/mg/dired.c ==
Go look the line 729:
if ((fopen(dname,"r")) == NULL) {
...
Now you can cry
