hercules-390 / hyperion

Hercules 390
Other
248 stars 67 forks source link

Console tty output incorrectly formatted #154

Closed PeterCoghlan closed 7 years ago

PeterCoghlan commented 7 years ago

Using the Multinet telnet client on VMS to connect to Hercules running on VMS, the tty console output appears stepped. Instead of each new line starting on the left hand edge of the screen, it starts in the position below where the previous line ended. I suspect the same issue may also arise with other telnet clients and other Hercules platforms. Using tcpdump to check the data on the network shows that each line is being terminated with a line feed only instead of a carriage return followed by a line feed as required by the standard. It is not necessary to ipl an operating system to see the effect - just to configure a 3215 device and telnet to it and it is apparant in in the three line banner which Hercules sends to the telnet client on receipt of a connection.

The issue does not arise when compiling from source retrieved before 28 August 2016 when three lines starting with "if (tn->devclass == 'K')" were removed from console.c. Prior to that point, everything seems to work fine. I tested using a tty console to log in to VM/370 and as a line mode console for MUSIC/SP. The output was formatted correctly in all cases and on the network, end of lines had carriage returns followed by line feeds as expected. The only slight issue that I noticed was that if the telnet client was abruptly disconnected when the operating system running on Hercules was not expecting this, a flurry of errors similar to the following appeared on the Hercules console:

19:59:24 HHC01022I 0:001F COMM: client 127.0.0.1 devtype 3215: connection closed by client
19:59:24 HHC01315I 0:001F CHAN: ccw 0AF0D9D8 60000086
19:59:24 HHC01312I 0:001F CHAN: stat 0E00, count 0086=>31383A35 393A3135 200A0000 00000000 ÉÎÑ..ÑÉ.½±......
19:59:24 HHC01313I 0:001F CHAN: sense 40000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
19:59:24 HHC01314I 0:001F CHAN: sense INTREQ
19:59:24 HHC02900E 0:001F COMM: Send() failed: broken pipe
19:59:24 HHC01315I 0:001F CHAN: ccw 0AF0D9D8 60000086
19:59:24 HHC01312I 0:001F CHAN: stat 0E00, count 0086=>404D5347 2046524F 4D204C4F 474F4E30  (..½Õ.×(½<×.×+È
19:59:24 HHC01313I 0:001F CHAN: sense 10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
19:59:24 HHC01314I 0:001F CHAN: sense EQC
19:59:25 HHC90507D 0:001F COMM: recv() failed: connection reset by peer
19:59:25 HHC01315I 0:001F CHAN: ccw 09F1B7E0 60000012=>15151540 E5D461F3 F7F040D6 D5D3C9D5 ... VM/370 ONLIN
19:59:25 HHC01312I 0:001F CHAN: stat 0200, count 0000
19:59:25 HHC01313I 0:001F CHAN: sense 40000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
19:59:25 HHC01314I 0:001F CHAN: sense INTREQ
19:59:25 HHC01315I 0:001F CHAN: ccw 0B000000 20000001
19:59:25 HHC01312I 0:001F CHAN: stat 0200, count 0001
19:59:25 HHC01313I 0:001F CHAN: sense 40000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
19:59:25 HHC01314I 0:001F CHAN: sense INTREQ

While the "recv() failed: connection reset by peer" error would appear only once as might be expected, the "Send() failed: broken pipe" error and subsequent three lines could be repeated approximately 30 times. This is not a big problem though. Overall, I think the new telnet server code is a big improvement on the old code which always seemed to be operating very close to the edge of what would work.

Can anyone else see the stepping effect or is there something about my telnet client that is provoking it? If I replace the three lines removed from console.c the stepping goes away.

jphartmann commented 7 years ago

With a Linux telnet client I get aligned output as it should be:

[/home/john/script/fpl/ug] telnet 0 3271 Trying 0.0.0.0... Connected to 0. Escape character is '^]'. HHC01027I Hercules version 4.0.0.8631-gda69c73, built on Sep 3 2016 02:47:39 HHC01031I Running on jack (Linux-3.13.0-93-generic.#140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 MP=4) HHC01018I 0:000A COMM: client 127.0.0.1 devtype 3215: connected

Given that Linux does not need a carriage return, this is no surprise.

But the CRLF is only a requirement for the NVT. I am not sure, but I doubt it applies this connexion. I'd have to go read the RFC.

On 09/03/2016 01:43 AM, PeterCoghlan wrote:

Using the Multinet telnet client on VMS to connect to Hercules running on VMS, the tty console output appears stepped. Instead of each new line starting on the left hand edge of the screen, it starts in the position below where the previous line ended. I suspect the same issue may also arise with other telnet clients and other Hercules platforms. Using tcpdump to check the data on the network shows that each line is being terminated with a line feed only instead of a carriage return followed by a line feed as required by the standard. It is not necessary to ipl an operating system to see the effect - just to configure a 3215 device and telnet to it and it is apparant in in the three line banner which Hercules sends to the telnet client on receipt of a connection.

The issue does not arise when compiling from source retrieved before 28 August 2016 when three lines starting with "if (tn->devclass == 'K')" were removed from console.c. Prior to that point, everything seems to work fine. I tested using a tty console to log in to VM/370 and as a line mode console for MUSIC/SP. The output was formatted correctly in all cases and on the network, end of lines had carriage returns followed by line feeds as expected. The only slight issue that I noticed was that if the telnet client was abruptly disconnected when the operating system running on Hercules was not expecting this, a flurry of errors similar to the following appeared on the Hercules console:

19:59:24 HHC01022I 0:001F COMM: client 127.0.0.1 devtype 3215: connection closed by client 19:59:24 HHC01315I 0:001F CHAN: ccw 0AF0D9D8 60000086 19:59:24 HHC01312I 0:001F CHAN: stat 0E00, count 0086=>31383A35 393A3135 200A0000 00000000 ÉÎÑ..ÑÉ.½±...... 19:59:24 HHC01313I 0:001F CHAN: sense 40000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 19:59:24 HHC01314I 0:001F CHAN: sense INTREQ 19:59:24 HHC02900E 0:001F COMM: Send() failed: broken pipe 19:59:24 HHC01315I 0:001F CHAN: ccw 0AF0D9D8 60000086 19:59:24 HHC01312I 0:001F CHAN: stat 0E00, count 0086=>404D5347 2046524F 4D204C4F 474F4E30 (..½Õ.×(½<×.×+È 19:59:24 HHC01313I 0:001F CHAN: sense 10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 19:59:24 HHC01314I 0:001F CHAN: sense EQC 19:59:25 HHC90507D 0:001F COMM: recv() failed: connection reset by peer 19:59:25 HHC01315I 0:001F CHAN: ccw 09F1B7E0 60000012=>15151540 E5D461F3 F7F040D6 D5D3C9D5 ... VM/370 ONLIN 19:59:25 HHC01312I 0:001F CHAN: stat 0200, count 0000 19:59:25 HHC01313I 0:001F CHAN: sense 40000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 19:59:25 HHC01314I 0:001F CHAN: sense INTREQ 19:59:25 HHC01315I 0:001F CHAN: ccw 0B000000 20000001 19:59:25 HHC01312I 0:001F CHAN: stat 0200, count 0001 19:59:25 HHC01313I 0:001F CHAN: sense 40000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 19:59:25 HHC01314I 0:001F CHAN: sense INTREQ

While the "recv() failed: connection reset by peer" error would appear only once as might be expected, the "Send() failed: broken pipe" error and subsequent three lines could be repeated approximately 30 times. This is not a big problem though. Overall, I think the new telnet server code is a big improvement on the old code which always seemed to be operating very close to the edge of what would work.

Can anyone else see the stepping effect or is there something about my telnet client that is provoking it? If I replace the three lines removed from console.c the stepping goes away.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/154, or mute the thread https://github.com/notifications/unsubscribe-auth/ABun09kHLo_4RFFnxo9IQtqUvFstUf0hks5qmLSigaJpZM4J0Kmh.

PeterCoghlan commented 7 years ago

As far as I understand it, the standard requires CRLF for the NVT and if the client and server negotiate a different terminal type, the requirement becomes whatever the terminal type negotiated requires. If this is a VT100 or compatible terminal type for example, CRLF is going to be needed at the end of each line because when a VT100 receives an LF only, it goes down one line and does not return to the left margin.

I've tried testing with a Linux telnet client. The output is displayed correctly in both the before and after cases so it appears the Linux client is indeed ignoring the carriage return if it is present. This is technically not a correct way to emulate a VT100 etc terminal but will suffice in many cases but not it seems for testing to ensure a telnet server is behaving correctly.

Irregardless of what happens for particular terminal types, correct NVT behaviour should be provided anyway in case the client and server do not negotiate a terminal type for whatever reason.

jphartmann commented 7 years ago

I concur.

Fish-Git commented 7 years ago

The issue does not arise when compiling from source retrieved before 28 August 2016 when three lines starting with if (tn->devclass == 'K') were removed from console.c. Prior to that point, everything seems to work fine.

Which was the change encouraged by Gordon Miller which I all too readily conceded to. I guess I shouldn't let others intimidate me into fixing things that aren't broken.

I'll back it out.

Aside: I still haven't located any Hercules code that properly null terminates the buffer before issuing the telnet_printf so Gordon's point is still valid. A potentially dangerous bug is still lurking. The only reason it's working today is due to the way libtelnet handles the telnet_printf call: it passes the request to telnet_vprintf which first does a vsnprintf (which effectively prevents any type of buffer overflow/overrun from occurring), processes the resulting formatted buffer character by character (automatically converting '\n' into '\r\n') and then uses its internal _send function to transmit the exact byte amount.

So while our libtelnet code is saving our fanny today that may not be true tomorrow, so IMO our existing constty_input function should be fixed to ensure its buffer is always null terminated so that when the sendto_clientfunction issues the telnet_printf call, we can be sure it will only print the data we want and not the entire buffer.

(But telnet_printf does need to be brought back. Give me a few minutes and I'll do that.)

Fish-Git commented 7 years ago

Hey. Shouldn't the telnet_printf call in sendto_client (see 'Aside' above) be done for printer devices too? (tn->devclass == P'?)

PeterCoghlan commented 7 years ago

I wonder should the telnet server be able to send out nulls in tty mode even though this probably doesn't happen in practice?

Maybe it would be better to not use null terminated strings here and manually convert /n to /r/n rather than calling telnet_printf() ?

I wonder do the the /n's get generated within Hercules anyway somewhere in the channel code when converting from EBCDIC to ASCII? If so, maybe it would be as easy to generate /r/n instead of /n at that point?

(I haven't looked into how much work it would be to change from null terminated strings to length specified strings though.)

Fish-Git commented 7 years ago

Maybe it would be better to not use null terminated strings here and manually convert /n to /r/n rather than calling telnet_printf() ?

No. That's libtelnet's job. Libtelnet was written to insulate servers from having to concern themselves about such things. They just do "printf" just like they normally would when issuing a message to stdout, and libtelnet converts newlines and carriage returns and embedded X'FF's, etc, according to the telnet protocol.

Trying to deal with all of that manually ourselves was how we got into our original mess in the first place which libtelnet got us out of. I don't feel going backwards is the correct/proper thing to do here.

At least that's my considered opinion on the matter.

jphartmann commented 7 years ago

Is libtelnet used for the NVT only? If not, the point about embedded nulls is a valid one.

Fish-Git commented 7 years ago

I disagree.

Yes libtelnet is obviously used not just for NVT. Duh. But that's not the point.

Any null terminating a string being printed is only there to prevent a printf from trying to print too much. The null itself would never be sent for NVT.

Fish-Git commented 7 years ago

Shouldn't the telnet_printf call in sendto_client (see 'Aside' above) be done for printer devices too? (tn->devclass == P'?)

Found my own answer. Quoting from page 4 of RFC 1646 "TN3270 Extensions for LUname and Printer Selection":

   Support for the TN3287 data stream requires that both sides:

      A.  Are able to exchange binary data.

      B. Can establish the agreement between client and server on the
         terminal type that will be used.

      C. Agree to use the TELNET IAC EOR as a delimiter for inbound
         and outbound TN3287 data streams

So using telnet_printf for TTY consoles only (tn->devclass == 'K') is correct.

PeterCoghlan commented 7 years ago

Fish, that's a very good point about it being libtelnet's job. However, if it is libtelnet's job, it appears we have to ensure what goes to libtelnet is null terminated and trust that libtelnet is always going to be there to do what is required of it. We have to forget about being able to send embedded nulls in tty mode (which is probably not a big deal anyway) as it appears that libtelnet does not provide a way of processing strings containing them while also inserting CRLF when required?

If we are going down this road, I wonder should we also check that any strings sent to telnet_printf (in tty mode) do not inadvertently contain embedded nulls as this will cause them to be truncated as far as telnet_printf is concerned?

I'm not sure null terminated strings are ever a good choice to contain something which could contain arbitrary characters generated by an operating system or program running on Hercules which knows only length specified format output and does not regard any particular characters as special but I can see the merit in relying on libtelnet to handle issues such as IACs and CRLFs and this seems to require their use here.

Fish-Git commented 7 years ago

We have to forget about being able to send embedded nulls in tty mode (which is probably not a big deal anyway) as it appears that libtelnet does not provide a way of processing strings containing them while also inserting CRLF when required?

Um, no. You can still send embedded nulls if you really want to by simply doing:

telnet_printf("An embedded null %c followed by some text.\n", '\0' );

But then why would you want to?

PeterCoghlan commented 7 years ago

Won't telnet_printf find the null between the words "null" and "followed" and be forced to conclude that this is the null which terminates the string? How can it know that there is another null further on?

ivan-w commented 7 years ago

I understand... That would fail...

But why would you want to insert a NULL (0x00) character in the middle of your (ASCII/Null terminated) datastream ?

The purpose of telnet_printf is not to send a pre-formatted 3278/3287 EBCDIC stream. The purpose of telnet_printf() is to send an ASCII/UTF8 string to a 1025/3215 (type K) device... 3278/3287 use telnet_send() (type D and P)

2703, 2741, 3705+3720+3725+3745/EP and such are treated somewhere else (but TELE-2 types should probably go through libtelnet too ! Not BSC since BSC is not intended to be used using telnet but a straight TCP connection between hercules and hercules or hercules and an RJE emulator).

On 9/3/2016 1:26 PM, PeterCoghlan wrote:

Won't telnet_printf find the null between the words "null" and "followed" and be forced to conclude that this is the null which terminates the string? How can it know that there is another null further on?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/154#issuecomment-244541237, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjMW-aRdNFJ3knU9t4yoZv5sFj82XzNks5qmVl5gaJpZM4J0Kmh.

Fish-Git commented 7 years ago

Won't telnet_printf find the null between the words "null" and "followed" and be forced to conclude that this is the null which terminates the string?

No. It doesn't work that way. The call to telnet_printf formats a string to be sent to the client. If the string contains embedded nulls, they WILL be sent. Libtelnet does not stop transmitting data when it sees a null character. It sends whatever the hell data you tell it to send, and if the data you want to send is a null character, so be it.

Please take a look at the code in telnet_vprintf. It does not stop when it reaches a null. It always send ALL of your data, embedded nulls and all.

PeterCoghlan commented 7 years ago

I wouldn't really want to insert a null in the datastream but my point is that something running on Hercules could generate one (either inadvertently or because of lazy programming or perhaps even for some purpose), being unaware that outputting a null in the middle of some other data could cause the truncation of the data when running on Hercules but not when running on the "real" hardware.

While the purpose of telnet_printf() is to send an ASCII/UTF8 null terminated string to a remote telnet client, what we are using it for is to send a string which is originally an EBCDIC encoded length specified string which can contain any characters to a remote telnet client.

Thinking about it some more, perhaps embedded nulls could be removed just before or just after the EBCDIC to ASCII translation?

If this is not done, I could imagine some strange, hard to debug issues where output does not appear because a previously unseen null was allowed to get into the datastream.

Fish-Git commented 7 years ago

I understand... That would fail...

No it would not.

Fish-Git commented 7 years ago

that outputting a null in the middle of some other data could cause the truncation of the data when running on Hercules

Wrong. Please re-read my earlier comment.

Fish-Git commented 7 years ago

While the purpose of telnet_printf() is to send an ASCII/UTF8 null terminated string to a remote telnet client,

That is incorrect too. The data you send -- whether it is to a NVT (tty) terminal or a 3270 terminal, it doesn't matter -- is NOT required to be null terminated!

If this is not done, I could imagine some strange, hard to debug issues where output does not appear because a previously unseen null was allowed to get into the datastream.

Will never occur. What would occur is you would see an "unusual" funny looking character in your client. Whatever message was sent that contained the embedded null would be sent in its entirety. Absolutely no truncation would occur.

ivan-w commented 7 years ago

On 9/3/2016 2:08 PM, Fish-Git wrote:

While the purpose of telnet_printf() is to send an ASCII/UTF8 null
terminated string to a remote telnet client,

That is incorrect too. The data you send -- whether it is to a NVT (tty) terminal or a 3270 terminal, it doesn't matter -- is /NOT/ required to be null terminated!

If this is not done, I could imagine some strange, hard to debug
issues where output does not appear because a previously unseen
null was allowed to get into the datastream.

Will never occur. What /would/ occur is you would see an "unusual" funny looking character in your client. Whatever message was sent that contained the embedded null would be sent in its /entirety/. Absolutely /no/ truncation would occur.

I stand corrected.

The return of vsnprintf (the string size) does rely on a null terminated string for the format, NOT the resulting string (even if contains a NULL). If the resulting string contains a NULL, it will not be truncated, since what is sent to the client is not a C terminated string but the contents of the buffer resulting from the vsnprintf.

My apologies,

--Ivan

jphartmann commented 7 years ago

As for embedded nulls, think of a 3270 structured field. Tinkering with the nulls would be disastrous.

ivan-w commented 7 years ago

On 9/3/2016 2:08 PM, Fish-Git wrote:

While the purpose of telnet_printf() is to send an ASCII/UTF8 null
terminated string to a remote telnet client,

That is incorrect too. The data you send -- whether it is to a NVT (tty) terminal or a 3270 terminal, it doesn't matter -- is /NOT/ required to be null terminated!

If this is not done, I could imagine some strange, hard to debug
issues where output does not appear because a previously unseen
null was allowed to get into the datastream.

Will never occur. What /would/ occur is you would see an "unusual" funny looking character in your client. Whatever message was sent that contained the embedded null would be sent in its /entirety/. Absolutely /no/ truncation would occur.

But however...

I can only see ONE invocation of telnet_printf() - that's in console.c sendto_client()

The only invocation is telnet_printf(tn->ctl,"%s",buf) (at line 650 with my latest GIT pull) (Note, there is no length) - which resolves to a vsnprintf() in telnet.c... So if there is a NULL in the original "buf", the vsnprintf will stop there.

So if I do a CCW to a 3215 console with an embedded NULL, the resulting output MAY be truncated as "buf" to telnet_printf is expected to be a null terminated C string (unless the EBCDIC->ASCII translates NO character from 0 to 0).

--Ivan

PeterCoghlan commented 7 years ago

Depending on what vsnprintf() does, it may be possible to get around the issue by generating a %c directive in the format string for each character to be sent and then sending the right number of arguments to match them but this is really going to extremes. Any embedded nulls in the format string or any following string arguments are going to result in truncation. The only way of avoiding this is to use a function like telnet_send() which takes a length argument.

I think we should forget about trying to transmit embedded nulls in tty mode and just remove them at some point before calling telnet_printf(), unless the providers of libtelnet can be persuaded to provide a function like telnet_send but which also does the required LF -> CRLF translations.

ivan-w commented 7 years ago

On 9/3/2016 2:43 PM, John P. Hartmann wrote:

As for embedded nulls, think of a 3270 structured field. Tinkering with the nulls would be disastrous.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/154#issuecomment-244544320, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjMW5PUS_2G0Uk5-ZlXcJdLWclzRADWks5qmWuEgaJpZM4J0Kmh.

There is no issue with 3270 formatted datastreams (they are sent as-is as a buffer and a length - and the eventual telnet control stuff which is inserted to comform with the tn3270 protocol). That's type 'D' and 'P'

The only issue I am wondering about is 3215 consoles. (type 'K')

--Ivan

ivan-w commented 7 years ago

On 9/3/2016 2:46 PM, PeterCoghlan wrote:

Depending on what vsnprintf() does, it may be possible to get around the issue by generating a %c directive in the format string for each character to be sent and then sending the right number of arguments to match them but this is really going to extremes. Any embedded nulls in the format string or any following string arguments are going to result in truncation. The only way of avoiding this is to use a function like telnet_send() which takes a length argument.

I think we should forget about trying to transmit embedded nulls in tty mode and just remove them at some point before calling telnet_printf(), unless the providers of libtelnet can be persuaded to provide a function like telnet_send but which also does the required LF -> CRLF translations.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/154#issuecomment-244544458, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjMW8P2pyNxDLYgyA_k-8HCXTmnJw42ks5qmWxDgaJpZM4J0Kmh.

What I am seeing under VM (3215 emulation on a 3270 terminal) : REW 181 REWIND COMPLETE I 181 EXPECTING AB OR A B : A B CP entered; disabled wait PSW '00020000 00000000'

(This is a control test with a 3215 over 3270 emulator that has been in operation and working for almost 50 years).

What I am seeing standalone under hercules with the integrated 3215-C console : HHC01603I devinit 580 tst3215.het HHC00221I 0:0580 Tape file tst3215.het, type het: format type Hercules Emulated Tape file HHC00224I 0:0580 Tape file tst3215.het, type het: display " NT RDY " HHC02245I 0:0580 device initialized HHC01603I iplc 580 /EXPECTING AB OR A B : A B HHC00809I Processor CP00: disabled wait state 00020000 80000000 (This is a test using a hercules 3215 emulator that does not rely on libtelnet)

What I am seeing standalone under hercules with a telnet attached 3215 terminal using PUTTY :

HHC01027I Hercules version 4.0.0.8636-g72a9399-modified, built on Sep 3 2016 15:07:56 HHC01031I Running on deb001 (Linux-3.16.0-4-amd64.#1 SMP Debian 3.16.7-ckt25-2+deb8u3 (2016-07-02) x86_64 MP=8) HHC01018I 0:0009 COMM: client 192.168.77.1 devtype 3215: connected *\ And after IPL : EXPECTING AB OR A B : A B

*\ Source code : type tst3215 assemble

TST321A CSECT ORG +X'20000' TST3215 DS 0H BALR 12,0 USING ,12 LA 1,TSTCCW ST 1,X'48' SIO X'9' LPSW DISAWAIT DS 0D TSTCCW CCW X'09',BFR,X'20',LBFR BFR DC C'EXPECTING AB OR A B : A',X'00',C'B' LBFR EQU -BFR DS 0D DISAWAIT DC X'00020000',X'00000000' END TST3215

IPL Tape built with :

type doiplt exec a

&CONTROL OFF NOMSG ERASE IPLFILE $TEMP$ A COPY 3CARD LOADER S2 IPLFILE $TEMP$ A COPY &1 TEXT A IPLFILE $TEMP$ A ( APPEND FILEDEF INMOVE DISK IPLFILE $TEMP$ A ( RECFM F LRECL 80 BLKSIZE 80 FILEDEF OUTMOVE TAP1 ( RECFM F LRECL 80 BLKSIZE 80 REWIND 181 MOVEFILE REWIND 118


So apparently there is NO issue with the 3215 support with data in the CCWs with embedded NULL characters.

False alarm !

--Ivan

Fish-Git commented 7 years ago

So if I do a CCW to a 3215 console with an embedded NULL, the resulting output MAY be truncated as "buf" to telnet_printf is expected to be a null terminated C string

Correct. But then that should be properly considered as to be a bug in Hercule, yes? I mean, there shouldn't be any embedded nulls in a 3215/tty ('K' console device) buffer, right? Such an occurrence should be considered an error.

Fish-Git commented 7 years ago

unless the providers of libtelnet can be persuaded to provide a function like telnet_send but which also does the required LF -> CRLF translations.

That would be ourselves. We could add such a function to our libtelnet implementation if we really wanted to. But again, is there really a legitimate bona fide need to send nulls to such devices?

jphartmann commented 7 years ago

If I do a CCW code 9 to my virtual console and the buffer contains a null, I shall expect a blank on the 3215. If I don't get that, it is a bug in the telnet server caused by the library. It sure as taxes is not a bug anywhere else in Hyperion.

Fish-Git commented 7 years ago

So apparently there is NO issue with the 3215 support with data in the CCWs with embedded NULL characters.

False alarm !

Interesting! I was actually expecting it to be truncated. Wow. I might have to step through the code line by line to see what's going on.

In any case it's good to see the correct thing occurring even if we don't know how it's occurring!  :)

Fish-Git commented 7 years ago

I shall expect a blank on the 3215.

Why? Is that the way a real 3215 behaves? Do 3215 devices (or plain TTY devices) perform such translations?

Me, I'd expect the null since that's what was sent. I don't like channel code or device code changing the data I specifically asked it to send.

ivan-w commented 7 years ago

On 9/3/2016 5:11 PM, Fish-Git wrote:

So apparently there is NO issue with the 3215 support with data in the
CCWs with embedded NULL characters.

False alarm !

Interesting! I was actually expecting it to be truncated. Wow. I might have to step through the code line by line to see what's going on.

In any case it's good to see the correct thing occurring even if we don't know /how/ it's occurring! |:)|

So did I, and that's why I did the test case ! Then the test case, unexpectedly came back as - correct ! (not truncated).

I'll look for it some more (I hate it when something is wrong and I can't find why (a bug).. I hate it more when I expect something to go wrong and doesn't (magic ?))

I HATE magic !

--Ivan

Fish-Git commented 7 years ago

I HATE magic !

Aye. Me too.  :)

jphartmann commented 7 years ago

Fish, the 1052 (selectric (golf ball) for /370), 3210 (selectric), 3215 (7-wire matrix) were not TTYs, but system console devices (though you could no doubt get terminals with the 3215 print mechanisms).

The devices were all channel attached.

They print a blank given a null. Or, I suppose they print nothing at all, which is just the same as long as the carriage is advanced.

https://en.wikipedia.org/wiki/File:IBM_System360_Model_30.jpg shows the 1052 in the foreground; the 1050 controller is below the tabletop. The model 30 was unique in /360 in having the controller outboard.

https://en.wikipedia.org/wiki/IBM_System/360#/media/File:IBM_system_360-50_console_-_MfK_Bern.jpg Better view of the 1052 front left.

https://en.wikipedia.org/wiki/File:IBM_370-145_2.png Computer generated image, but pretty good. The 3215 is at the right.

Fish-Git commented 7 years ago

https://en.wikipedia.org/wiki/File:IBM_System360_Model_30.jpg shows the 1052 in the foreground; the 1050 controller is below the tabletop. The model 30 was unique in /360 in having the controller outboard.

I'm quite familiar with the model 30. It was the first mainframe I ever used. I learned 360 assembler on it. :)

They print a blank given a null. Or, I suppose they print nothing at all, which is just the same as long as the carriage is advanced.

Okaaay... Good point.

Then maybe we should have some type of function to pre-process the buffer before issuing the telnet_printf call. Perhaps in sendto_client just before we call telnet_printf?

And should we decide to do that then IMO we should at least issue some type of warning message that it is occurring since I don't believe any well behaved guest operating systems should really ever be doing that!

jphartmann commented 7 years ago

Fish, there is nothing magic about x'00' in the /360 world, except that (a) it is rejected as a CCW code because the channel uses opcode x'00' for its own devious purpose (TIO, in fact); and (b) as an operation code, but this was announced only just recently after intense pressure from (would you believe it?) CICS.

No diagnostic needed, no spanking of the insolent pupil: x'00' is A-OK!

As far as what should be done, I suppose I shall have to make another test to see whether there is an issue, just to confirm Ivan's result. I already have everything except the program ready, so that should be easy enough. But hey, it is Saturday night here.

ivan-w commented 7 years ago

On 9/3/2016 7:09 PM, John P. Hartmann wrote:

Fish, there is nothing magic about x'00' in the /360 world, except that (a) it is rejected as a CCW code because the channel uses opcode x'00' for its own devious purpose (TIO, in fact); and (b) as an operation code, but this was announced only just recently after intense pressure from (would you believe it?) CICS.

John,

We were talking about DATA in the 3215 Write CCW ! Not the CCW opcode !

--Ivan

jphartmann commented 7 years ago

Read it again, Ivan, I was enumerating the two places where x'00' will be rejected. The inference is that it is OK anywhere else.

Fish-Git commented 7 years ago

No diagnostic needed, no spanking of the insolent pupil: x'00' is A-OK!

That suits me even better! :)

ivan-w commented 7 years ago

John,

We were NEVER talking about an operation code or CCW code.

We were merely talking about a DATA stream being sent to a 3215 on a X'09' CCW code and how it might be converted (or truncated) to a telnet session supporting a 3215 session if there was ever a 0x00 byte embedded within the I/O buffer being sent to a 3215 (we were worried it might cut off the rest of the streams being sent to the telnet session due to C functions assuming a 0x00 byte indicating the end of a string).

I am not sure how X'00' I/O initiation code (TIO) or CICS may have any relevancy here.

--Ivan

On 9/3/2016 7:21 PM, John P. Hartmann wrote:

Read it again, Ivan, I was enumerating the two places where x'00' will be rejected. The inference is that it is OK anywhere else.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/154#issuecomment-244558701, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjMW2m88uZr-P2OI2DoHhZarL6geXFuks5qmayPgaJpZM4J0Kmh.

jphartmann commented 7 years ago

Perhaps, Ivan, but if it suits Fish, I think it should suit you too.

ivan-w commented 7 years ago

On 9/3/2016 7:34 PM, John P. Hartmann wrote:

Perhaps, Ivan, but if it suits Fish, I think it should suit you too.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hercules-390/hyperion/issues/154#issuecomment-244559408, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjMW3fu2C95_V9aMi-FQKU1g2ps4ewIks5qma-mgaJpZM4J0Kmh.

John,

The wonder is that.. Looking at the code, the CCW data SHOULD have been truncated... But it's not !

Preliminary code analysis says the output should be truncated but experiments shows it is not.

Hence the 'We do not like magic" cry ! So we obviously missed something in something.

Still looking into it !

... There are times when you look at the code, and looking at it shows it shouldn't work - but when the program runs, it does... Then there is something wrong - let's called that an Anti-Bug !

--Ivan

PeterCoghlan commented 7 years ago

I've run Ivan's test with Hercules running in the debugger. This showed up that in constty_execute_ccw(), at the point where the data is translated from EBCDIC to ASCII, each character is also checked for being a printable character, a line feed or a carriage return and if neither of these, it is replaced by a space. Nulls will be replaced by spaces here.

After that, a line feed is added. No terminating null is added. The data is still in length specified format and is passed to sendto_client().

In the case of 1052/3215 tty devices, sendto_client() then passes the data to telnet_printf()

Perhaps adding a null terminator in sendto_client(), immediately prior to the call to telnet_printf() after checking there is space available for it in the buffer would be a good interim solution?

I suspect that some of us have been lucky enough to already have zeros at the end of our buffers but perhaps Gordon Miller, who originally reported the problem of "garbled characters" and "spurious garbage", was not as lucky and telnet_printf() picked up some random characters at the end of his buffer, up to wherever there was a null?

Fish-Git commented 7 years ago

Perhaps adding a null terminator in sendto_client(), immediately prior to the call to telnet_printf() after checking there is space available for it in the buffer would be a good interim solution?

Can't. buf is declared const in sendto_client.

Instead, we just need to tweak the constty_execute_ccw code that processes opcodes 0x01 and 0x09 (as well as 0x0B too of course) to ensure the buffer it passes to sendto_client is always null terminated, which I have now done. Can we put this issue to rest now?

jphartmann commented 7 years ago

CCW opcode x'0B' is the audible alarm; an immediate command that does not transfer data.

Fish, did you ever ring that bell? I shook the entire building. The bell itself was more than a foot across.

Fish-Git commented 7 years ago

CCW opcode x'0B' is the audible alarm; an immediate command that does not transfer data.

On a real machine, sure. But on Hercules, if your 1052 or 3215 system console is a telnet client we need to send an ANSI bell code ('\a') to the connected client so it can ring the bell.

Fish, did you ever ring that bell? It shook the entire building. The bell itself was more than a foot across.

I can't recall that I ever did, no. Never saw one either. If it was indeed a foot across I don't doubt that it shook the entire building!

PeterCoghlan commented 7 years ago

Fish,

I've tested out the latest fix and it looks good to me. The output is correctly formatted and not stepped as I previously experienced.

I never experienced any of the "garbled characters" and "spurious garbage" reported elsewhere but I am confident that this fix addresses that issue and will prevent it from recurring.

Many thanks.