PATCH: Re: Segmentation fault 3.1.7-pre-3/4

PATCH: Re: Segmentation fault 3.1.7-pre-3/4

[Sven Wischnowsky]

Bernd Eggink wrote:

> This gives a seg fault if executed in a script:
> 
>     read -q "REPLY?hm: "
> 
> Happens with pre-3 and pre-4, not with pre-1.

Oh, look Bart, there's the reason to not always use shout (11036).

The patch tries to make sure we're always on the save side. I
hope. It's different from the pre-11036 state.

But with zle not loaded, the

	zsh -c "read -q '?Can you see this? '" < /dev/null >& /dev/null

from 11036 doesn't show the prompt (but at least it doesn't segv
anymore). Is it right or wrong? Do we have to work around it?

Bye
 Sven

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.18
diff -u -r1.18 builtin.c
--- Src/builtin.c	2000/05/19 15:10:54	1.18
+++ Src/builtin.c	2000/05/23 08:30:31
@@ -3440,8 +3440,8 @@
 	     *readpmpt && *readpmpt != '?'; readpmpt++);
 	if (*readpmpt++) {
 	    if (keys || isatty(0)) {
-		zputs(readpmpt, shout);
-		fflush(shout);
+		zputs(readpmpt, (shout ? shout : stderr));
+		fflush(shout ? shout : stderr);
 	    }
 	    readpmpt[-1] = '\0';
 	}

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: Segmentation fault 3.1.7-pre-3/4

[Peter Stephenson]

Sven wrote
> But with zle not loaded, the
> 
> 	zsh -c "read -q '?Can you see this? '" < /dev/null >& /dev/null
> 
> from 11036 doesn't show the prompt (but at least it doesn't segv
> anymore). Is it right or wrong? Do we have to work around it?

Seems to me that after doing your damnedest *not* to have the prompt shown,
you don't want it.  `read -q' is documented as always reading from
the terminal, and the prompt as printing on stderr.  I haven't checked
that's what's actually going on, but it's consistent.

I worry a bit about having a hidden file descriptor and FILE * in general:
we should be sure that the shell only uses them for editing or where
explicitly indicated.  But I think it's OK here.

-- 
Peter Stephenson <pws@cambridgesiliconradio.com>
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: PATCH: Re: Segmentation fault 3.1.7-pre-3/4

[Bart Schaefer]

On May 23, 10:31am, Sven Wischnowsky wrote:
} Subject: PATCH: Re: Segmentation fault 3.1.7-pre-3/4
}
} Bernd Eggink wrote:
} 
} > This gives a seg fault if executed in a script:
} > 
} >     read -q "REPLY?hm: "
} > 
} > Happens with pre-3 and pre-4, not with pre-1.
} 
} Oh, look Bart, there's the reason to not always use shout (11036).

Hmm, why did I think that SHTTY != -1 mean that shout was nonzero?

I personally would prefer the following patch, but I haven't committed
it yet.

Index: builtin.c
===================================================================
@@ -4970,6 +4970,10 @@
 		    oshout = shout;
 		    init_shout();
 		}
+	    } else if (!shout) {
+		haso = 1;
+		oshout = shout;
+		init_shout();
 	    }
 	    /* We should have a SHTTY opened by now. */
 	    if (SHTTY == -1) {



On May 23, 10:10am, Peter Stephenson wrote:
} Subject: Re: PATCH: Re: Segmentation fault 3.1.7-pre-3/4
}
} Sven wrote
} > But with zle not loaded, the
} > 
} > 	zsh -c "read -q '?Can you see this? '" < /dev/null >& /dev/null
} > 
} > from 11036 doesn't show the prompt (but at least it doesn't segv
} > anymore). Is it right or wrong? Do we have to work around it?
} 
} Seems to me that after doing your damnedest *not* to have the prompt shown,
} you don't want it.  `read -q' is documented as always reading from
} the terminal, and the prompt as printing on stderr.  I haven't checked
} that's what's actually going on, but it's consistent.

It's confusing.  If I've written a script and embedded a "read -q" in it
somewhere, chances are I expect it to display the prompt and not just
hang mysteriously until the user happens to touch the keyboard.  It should
not be necessary to always write something like

	[[ -t 2 ]] && read -q '?Can you see this? ' 2>/dev/tty

because the shell knows much better than I do whether there *is* a tty.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: Re: Segmentation fault 3.1.7-pre-3/4

[Bart Schaefer]

On May 23,  3:44pm, Bart Schaefer wrote:
} Subject: Re: PATCH: Re: Segmentation fault 3.1.7-pre-3/4
}
} I personally would prefer the following patch, but I haven't committed
} it yet.

And a good thing, too, as I just realized that it will cause SHTTY to be
closed improperly.

Sigh.

} +	    } else if (!shout) {
} +		haso = 1;
} +		oshout = shout;
} +		init_shout();
}  	    }

What horrible things would happen if `haso' were simply left as 0 here?
Then `shout' would remain open, but as it's only pointing to the fd that
is already open for SHTTY, and no other part of the code ever tests shout
for non-zero-ness (except init_io() which clobbers SHTTY anyway), there
doesn't seem to be any problem except the tiny overhead of one extra FILE
structure.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: Re: Segmentation fault 3.1.7-pre-3/4

[Bart Schaefer]

On May 23,  4:10pm, Bart Schaefer wrote:
} Subject: Re: PATCH: Re: Segmentation fault 3.1.7-pre-3/4
}
} } +	    } else if (!shout) {
} } +		haso = 1;
} } +		oshout = shout;
} } +		init_shout();
} }  	    }
} 
} What horrible things would happen if `haso' were simply left as 0 here?

No one has had anything to say about this, so I've committed the following
patch.

Index: builtin.c
===================================================================
@@ -3406,6 +3406,9 @@
 		    oshout = shout;
 		    init_shout();
 		}
+	    } else if (!shout) {
+		/* We need an output FILE* on the tty */
+		init_shout();
 	    }
 	    /* We should have a SHTTY opened by now. */
 	    if (SHTTY == -1) {

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com