Goodbye to you my file descriptor

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Goodbye to you my file descriptor

Maxime Villard
MMmhh...

== /usr/src/usr.bin/mg/dired.c ==
Go look the line 729:

    if ((fopen(dname,"r")) == NULL) {
        ...

Now you can cry

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
On 30 October 2012 14:36, rustyBSD <[hidden email]> 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 ?

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Mike Belopuhov-5
On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
<[hidden email]> wrote:

> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
In reply to this post by Christiano F. Haesbaert
On 30 October 2012 14:58, Christiano F. Haesbaert
<[hidden email]> wrote:

> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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 :).

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
In reply to this post by Mike Belopuhov-5
On 30 October 2012 15:00, Mike Belopuhov <[hidden email]> wrote:

> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
> <[hidden email]> wrote:
>> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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 !

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
On 30 October 2012 15:03, Christiano F. Haesbaert
<[hidden email]> wrote:

> On 30 October 2012 15:00, Mike Belopuhov <[hidden email]> wrote:
>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>> <[hidden email]> wrote:
>>> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Maxime Villard
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.c    Wed Mar 14 14:56:35 2012
+++ dired.c    Tue 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);

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Okan Demirmen
In reply to this post by Christiano F. Haesbaert
On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
<[hidden email]> wrote:

> On 30 October 2012 15:03, Christiano F. Haesbaert
> <[hidden email]> wrote:
>> On 30 October 2012 15:00, Mike Belopuhov <[hidden email]> wrote:
>>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>>> <[hidden email]> wrote:
>>>> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
On 30 October 2012 16:45, Okan Demirmen <[hidden email]> wrote:

> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
> <[hidden email]> wrote:
>> On 30 October 2012 15:03, Christiano F. Haesbaert
>> <[hidden email]> wrote:
>>> On 30 October 2012 15:00, Mike Belopuhov <[hidden email]> wrote:
>>>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>>>> <[hidden email]> wrote:
>>>>> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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).

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
On 30 October 2012 16:52, Christiano F. Haesbaert
<[hidden email]> wrote:

> On 30 October 2012 16:45, Okan Demirmen <[hidden email]> wrote:
>> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
>> <[hidden email]> wrote:
>>> On 30 October 2012 15:03, Christiano F. Haesbaert
>>> <[hidden email]> wrote:
>>>> On 30 October 2012 15:00, Mike Belopuhov <[hidden email]> wrote:
>>>>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>>>>> <[hidden email]> wrote:
>>>>>> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Okan Demirmen
On Tue, Oct 30, 2012 at 11:53 AM, Christiano F. Haesbaert
<[hidden email]> wrote:

> On 30 October 2012 16:52, Christiano F. Haesbaert
> <[hidden email]> wrote:
>> On 30 October 2012 16:45, Okan Demirmen <[hidden email]> wrote:
>>> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
>>> <[hidden email]> wrote:
>>>> On 30 October 2012 15:03, Christiano F. Haesbaert
>>>> <[hidden email]> wrote:
>>>>> On 30 October 2012 15:00, Mike Belopuhov <[hidden email]> wrote:
>>>>>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>>>>>> <[hidden email]> wrote:
>>>>>>> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
On 30 October 2012 16:57, Okan Demirmen <[hidden email]> wrote:

> On Tue, Oct 30, 2012 at 11:53 AM, Christiano F. Haesbaert
> <[hidden email]> wrote:
>> On 30 October 2012 16:52, Christiano F. Haesbaert
>> <[hidden email]> wrote:
>>> On 30 October 2012 16:45, Okan Demirmen <[hidden email]> wrote:
>>>> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
>>>> <[hidden email]> wrote:
>>>>> On 30 October 2012 15:03, Christiano F. Haesbaert
>>>>> <[hidden email]> wrote:
>>>>>> On 30 October 2012 15:00, Mike Belopuhov <[hidden email]> wrote:
>>>>>>> On Tue, Oct 30, 2012 at 2:58 PM, Christiano F. Haesbaert
>>>>>>> <[hidden email]> wrote:
>>>>>>>> On 30 October 2012 14:36, rustyBSD <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

William Ahern-2
In reply to this post by Okan Demirmen
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
> <[hidden email]> wrote:
> > On 30 October 2012 16:52, Christiano F. Haesbaert
> > <[hidden email]> wrote:
> >> On 30 October 2012 16:45, Okan Demirmen <[hidden email]> wrote:
> >>> On Tue, Oct 30, 2012 at 10:32 AM, Christiano F. Haesbaert
> >>> <[hidden email]> wrote:
> >>>> On 30 October 2012 15:03, Christiano F. Haesbaert
> >>>> <[hidden email]> wrote:
<snip>

> >>>> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Christiano F. Haesbaert
In reply to this post by Maxime Villard
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.c    Wed Mar 14 14:56:35 2012
> +++ dired.c    Tue 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 -0000 1.51
+++ dired.c 3 Nov 2012 14:47:18 -0000
@@ -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);

Reply | Threaded
Open this post in threaded view
|

Re: Goodbye to you my file descriptor

Loganaden Velvindron-2
Thanks for fixing my mistake :-)


On Sat, Nov 3, 2012 at 6:57 PM, Christiano F. Haesbaert
<[hidden email]> 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.c    Wed Mar 14 14:56:35 2012
>> +++ dired.c    Tue 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 -0000      1.51
> +++ dired.c     3 Nov 2012 14:47:18 -0000
> @@ -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 !