Bug 18103

Summary: [PATCH] FTP_PASSIVE_MODE and libftpio
Product: Base System Reporter: efutch <efutch>
Component: binAssignee: freebsd-bugs (Nobody) <bugs>
Status: Closed FIXED    
Severity: Affects Only Me    
Priority: Normal    
Version: 4.0-STABLE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
file.diff none

Description efutch 2000-04-20 04:20:00 UTC 
I was just digging around in the libftpio source code.  It does only check
to see if the FTP_PASSIVE_MODE environment variable is defined at all.  I
made this small patch to it that I think makes enough sense almost to have
it comitted.  With this patch it makes libftpio also understand the
YES or NO value of FTP_PASSIVE_MODE.  It's a diff against
src/lib/libftpio.c rev 1.33 from 4.0-STABLE.  Check out this patch and let
me know if it works for you.  The if statements could probably be cleaned
up a little :), but it works.

It was in reference to a problem someone mentioned on the freebsd-mobile mailing list.
I think this would help people who expect FTP_PASSIVE_MODE=NO to acutally
NOT use passive mode :)

Sean O'Connell <sean@stat.Duke.EDU> mentions:
This might be worthy of a PR.  The way fetch currently works is
a bit wrong (certainly violates POLA).

So here it is :)

Fix: Apply this patch.  Patch can also be found at:
http://quake.nyct.net/~efutch/FreeBSD/ftpio.c.patch
How-To-Repeat: (tcsh) setenv FTP_PASSIVE_MODE=NO
fetch -v ftp://someurl.com/something
And notice that it will use passive ftp no matter what.
Comment 1 mheffner 2000-04-20 05:58:05 UTC 
On 20-Apr-2000 efutch@nyct.net wrote:
  | Apply this patch.  Patch can also be found at:
  | http://quake.nyct.net/~efutch/FreeBSD/ftpio.c.patch
  | 
  | Index: src/lib/libftpio/ftpio.c
  | ===================================================================
  | RCS file: /home/ncvs/src/lib/libftpio/ftpio.c,v
  | retrieving revision 1.33
  | diff -u -r1.33 ftpio.c
  | --- src/lib/libftpio/ftpio.c    1999/09/28 13:33:13     1.33
  | +++ src/lib/libftpio/ftpio.c    2000/04/20 02:28:20
  | @@ -498,8 +498,13 @@
  |  static void
  |  check_passive(FILE *fp)
  |  {
  | -    if (getenv("FTP_PASSIVE_MODE"))
  | -       ftpPassive(fp, TRUE);
  | +    char *cp;
  | +    cp = getenv("FTP_PASSIVE_MODE");
  | +    printf("debug: cp(%s)",cp);
  | +    if (!cp || (cp && (strncmp(cp,"NO",2)==0)))
  | +        ftpPassive(fp, FALSE);
  | +    if (cp && (strncmp(cp,"YES",3)==0))
  | +        ftpPassive(fp, TRUE);
  |  }
  |  
 
Maybe it should do the comparison without depending on the string case?

How about this:

{
        char *cp;
        cp = getenv("FTP_PASSIVE_MODE");
        if (cp && strncasecmp(cp, "YES", 3)==0)
                ftpPassive(fp, TRUE);
        else
                ftpPassive(fp, FALSE);
}


/****************************************
 * Mike Heffner <spock@techfour.net>    *
 * Fredericksburg, VA      ICQ# 882073  *
 * Sent at: 20-Apr-2000 -- 00:37:28 EST *
 * http://my.ispchannel.com/~mheffner   *
 ****************************************/
Comment 2 efutch 2000-04-20 06:22:37 UTC 
That looks better than my method.  I knew it could have been cleaned
up.  Here's is a new version of the patch.  You can also find it at
http://quake.nyct.net/~efutch/FreeBSD/ftpio.c.patch-2

[snip here]
Index: src/lib/libftpio/ftpio.c
===================================================================
RCS file: /home/ncvs/src/lib/libftpio/ftpio.c,v
retrieving revision 1.33
diff -u -r1.33 ftpio.c
--- src/lib/libftpio/ftpio.c    1999/09/28 13:33:13     1.33
+++ src/lib/libftpio/ftpio.c    2000/04/20 05:19:15
@@ -498,8 +498,12 @@
 static void
 check_passive(FILE *fp)
 {
-    if (getenv("FTP_PASSIVE_MODE"))
-       ftpPassive(fp, TRUE);
+    char *cp;
+    cp = getenv("FTP_PASSIVE_MODE");
+    if (cp && strncasecmp(cp, "YES", 3)==0)
+        ftpPassive(fp, TRUE);
+    else
+        ftpPassive(fp, FALSE);
 }
 
 static void
[snip here]

Thanks!

--
Eric Futch              New York Connect.Net, Ltd.
efutch@nyct.net         Technical Support Staff
http://www.nyct.net     (212) 293-2620
"Bringing New York The Internet Access It Deserves"


On Thu, 20 Apr 2000, Mike Heffner wrote:

>
>On 20-Apr-2000 efutch@nyct.net wrote:
>  | Apply this patch.  Patch can also be found at:
>  | http://quake.nyct.net/~efutch/FreeBSD/ftpio.c.patch
>  | 
>  | Index: src/lib/libftpio/ftpio.c
>  | ===================================================================
>  | RCS file: /home/ncvs/src/lib/libftpio/ftpio.c,v
>  | retrieving revision 1.33
>  | diff -u -r1.33 ftpio.c
>  | --- src/lib/libftpio/ftpio.c    1999/09/28 13:33:13     1.33
>  | +++ src/lib/libftpio/ftpio.c    2000/04/20 02:28:20
>  | @@ -498,8 +498,13 @@
>  |  static void
>  |  check_passive(FILE *fp)
>  |  {
>  | -    if (getenv("FTP_PASSIVE_MODE"))
>  | -       ftpPassive(fp, TRUE);
>  | +    char *cp;
>  | +    cp = getenv("FTP_PASSIVE_MODE");
>  | +    printf("debug: cp(%s)",cp);
>  | +    if (!cp || (cp && (strncmp(cp,"NO",2)==0)))
>  | +        ftpPassive(fp, FALSE);
>  | +    if (cp && (strncmp(cp,"YES",3)==0))
>  | +        ftpPassive(fp, TRUE);
>  |  }
>  |  
> 
>Maybe it should do the comparison without depending on the string case?
>
>How about this:
>
>{
>        char *cp;
>        cp = getenv("FTP_PASSIVE_MODE");
>        if (cp && strncasecmp(cp, "YES", 3)==0)
>                ftpPassive(fp, TRUE);
>        else
>                ftpPassive(fp, FALSE);
>}
>
>
>/****************************************
> * Mike Heffner <spock@techfour.net>    *
> * Fredericksburg, VA      ICQ# 882073  *
> * Sent at: 20-Apr-2000 -- 00:37:28 EST *
> * http://my.ispchannel.com/~mheffner   *
> ****************************************/
>
Comment 3 efutch 2000-04-20 17:56:05 UTC 
The final revision of the patch can be found at:
http://quake.nyct.net/~efutch/FreeBSD/ftpio.c.patch-3

Thanks

--
Eric Futch              New York Connect.Net, Ltd.
efutch@nyct.net         Technical Support Staff
http://www.nyct.net     (212) 293-2620
"Bringing New York The Internet Access It Deserves"
Comment 4 gdinolt 2000-04-30 21:02:53 UTC 
I too have been recently "bitten" by this problem. I suggest that the
documentation of FTP_PASSIVE_MODE in libftpio be fixed to describe what
the possible legal values are and that the various programs which use
libftpio either copy the same information or refer back to it. Here is a
possible fix to the the ftpio documentation

The following patch might be appropriate for ftpio.3

*** /usr/src/lib/libftpio/ftpio.3       Thu Mar  2 18:16:58 2000
--- ftpio.3     Sun Apr 30 12:38:04 2000
***************
*** 194,200 ****
  .Tn FTP
  connection.
  .It Ev FTP_PASSIVE_MODE
! Force the use of passive mode
  .Tn FTP .
  .El
  .Sh BUGS
--- 194,202 ----
  .Tn FTP
  connection.
  .It Ev FTP_PASSIVE_MODE
! If set and is not zero and does not match the regular expression
! .Em {Nn}{Oo}
! force the use of passive mode
  .Tn FTP .
  .El
  .Sh BUGS


With this patch to the documentation one might want to change the patch
to the code in ftpio.c as follows:

*** /usr/src/lib/libftpio/ftpio.c       Sun Apr 23 20:19:53 2000
--- ftpio.c     Sun Apr 30 12:57:47 2000
***************
*** 499,506 ****
  check_passive(FILE *fp)
  {
      char *cp = getenv("FTP_PASSIVE_MODE");
!
!     ftpPassive(fp, (cp && !strncmp(cp, "YES", 3)));
  }

  static void
--- 499,506 ----
  check_passive(FILE *fp)
  {
      char *cp = getenv("FTP_PASSIVE_MODE");
!     /* str routines return zero if comparisons  are true */
!     ftpPassive(fp,  cp &&  strncasecmp(cp, "no", 2) && strcmp(cp, "0")
);
  }

  static void


This business of setting "logical' nvironment variables "rationally" is
a pain at best. I suspect athat it would be nice to have a system
standard for such things, but we would spend years arguing about it.

Regards, George Dinolt
Comment 5 Dag-Erling Smørgrav freebsd_committer freebsd_triage 2000-05-28 10:34:31 UTC 
State Changed
From-To: open->closed

Fixed in rev. 1.35 of src/lib/libftpio/ftpio.c. 
The correct behaviour is to default to passive mode.