104529 - PR_fprintf I/O function misbehaves with standard output

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Julien Pierre]

On OS/2, when writing directly to standard output fd, writing \n goes to the
next line but also keeps the cursor at the same column as it was. As a result,
everything is garbled in command-line programs using NSPR. Eg. the following code :
<pre>
	PR_fprintf(PR_STDOUT,
"\nNetscape Cryptographic Module Utility\n"
"Usage: modutil [command] [options]\n\n"
"                            COMMANDS\n"

</pre>
produces the following output
<pre>


Netscape Cryptographic Module Utility
                                     Usage: modutil [command] [options]


                   COMMANDS
</pre>
Basically all \n need to be converted to \n\r in order to let the cursor return
to the beginning of the line before writing further.

Comment 1

[Julien Pierre]

Attachment: proposed patch

Comment 2

[Julien Pierre]

I'm just realizing that this may not be the the appropriate patch since multiple
printf will have the effect of doing the conversion over again. In addition, all
printf will do the \n to \n\r , conversion which is really only necessary for
writes to stdout & stderr - ie, writes to the console.
Some applications that use PR_sprintf to put strings in temporary buffers will
be different on OS/2 as a result.

The proper fix is probably an NSPR layer pushed on the _pr_stdout and pr_stderr
PRFileDesc* , so the conversion is only done when wanted. Wan-Teh, do you agree ?

Comment 3

[Mike Kaply [:mkaply]]

I don't understand what the actual source of the bug is though.

I don't see it as being an OS/2 problem because

fprintf(stdout, "hello\n");
fprintf(stdout, "hello\n");

works fine.

What about on Windows? \n on Windows should have the same behavior as OS/2.

Comment 4

[Wan-Teh Chang]

Michael,

Julien was talking about PR_fprintf, not fprintf.

Comment 5

[Julien Pierre]

Mike,

When you do a PR_fprintf to stdout, the underlying NSPR call goes to NSPR
PR_Write, which does a DosWrite directly to the file handle . I think the
problem comes from the way the stdout device on OS/2 interprets the \n and \r
characters.

When you go through the runtime library (fprintf) it somehow must be translating
the stream before writing it to the actual stdout file handle. If you have the
source to the C RTL you might be able to see that. It doesn't ship with VACPP as
far as I know, though.

Comment 6

[Mike Kaply [:mkaply]]

OK, I wrote a testcase and DosWrite and WriteFile definitely treat \n 
differently.

Maybe we should add special casing in the OS/2 code that just checks if the 
destination handle is 1 (stdout) and does the conversion then?

Comment 7

[Julien Pierre]

Mike,

I have attached a test case also that shows the differences in OS/2, NT and 
Unix.

Basically NT and Unix automatically translate the \n written to stdout to mean 
"carriage return + line feed". In OS/2 the device does not translate the \n to 
anything so it just skips a line without returning to the beginning of the line 
- that's a line feed without carriage return, like on a typewriter.

I think is it difficult to decide how to deal with this problem within the 
framework of the current NSPR file API, which does not distinguish between text 
and binary modes. Everything is always treated as binary by NSPR currently.

There are a number of places where the fix could go :

1) translate \n to \r\n within the printf functions as I originally proposed.
This is wrong because one could do several iterations of sprintf(newstring, 
"%s", oldstring) , thereby incurring more than one translation, and extra blank 
lines.

2) translate \n to \r\n in a PRIOLayer that would be pushed only on the stdin, 
stdout and stderr layers. Basically, this will trap calls to things like PR_Read 
and PR_Write, and add the proper translation between the input/output/error 
pipes and the application.

The problem with doing that is that it will break any application that try to 
input or output binary data with PR_Write . There is no NSPR API specified today 
to toggle the translation on/off - one would have to explicitly push or pop the 
layer on the fds in the application for OS/2 only, which is ugly and 
non-portable.

3) translate \n to \r\n in PR_sscanf and PR_fprintf

This would affect all PRFileDesc, not just _pr_stdin, _pr_stdout and _pr_stderr 
. My guess is that this is the closest to what the OS/2 C runtime library does 
currently. Since sscanf and fprintf always deal with text, the change shouldn't 
impact any applications using binary data. However, it will introduce a 
difference in the content of text files created by NSPR applications on an OS/2 
platform vs text files created by NSPR applications on a Unix and NT platform. 
Also, some files created on other platforms may not be read properly on OS/2, 
and vice-versa.

I'm afraid this could cause problems with applications that use temporary files 
as text, write them with PR_fprintf, and then send them to the network. In that 
case it could show some odd problems.

As you see none of the proposed fixes are perfect. If you have better ideas, let 
me know. Wan-Teh, please feel free to jump in and comment on this issue.

Comment 8

[Julien Pierre]

Attachment: test case showing differences in standard output handling accross platform

Comment 9

[Julien Pierre]

Just FYI the output of the program on Unix and NT is :

hello
world

and on OS/2
hello
     world

Comment 10

[Julien Pierre]

Talked to Wan-Teh.

He suggested we use some environment variables to set translation on stdin 
/stderr / stdout fds . The implementation would probably be in a PRIOLayer that 
would be pushed only on those fds.

However, I just realized that using environment variables has drawbacks. For 
example if one has an NSPR text-mode application, and an NSPR binary compression 
stream application, and wants to do :
nsprtextapp | nsprzipapp > test.zip

In that case we want the output from nsprtextapp to be translated, and the 
output from nsprzipapp to be binary.

Ultimately I think the application needs to know what it is doing - whether it 
is text or binary I/O, and should have a way of specifying it.
The problem is that there is no such concept in NSPR.

Here is a proposal to change that :

I suggest that a "translation" bit could be added in the public PRFileDesc 
structure. To preserve compatibility (and performance) with all existing NSPR 
applications, the default would be binary mode.

The application could then set the translation mode by a call to 
PR_SetTextFileTranslation(PRFileDesc*) . This function might return an error 
code if the descriptor does not support translation (eg. we probably don't care 
to do this on sockets or other types of PRFileDesc*, only on actual files).

It would also be possible to pass PR_OTEXT as one of the flags to PR_Open to 
open a file in translated mode .

Environment variables for stdin/stdout/stderr translation could still exist so 
that quick tests could be done on existing NSPR applications that don't call 
PR_SetTextFileTranslation on the _pr_stdin / _pr_stdout _pr_stderr that are 
returned by PR_GetSpecialFD , but ultimately the application code should be 
modified and these variables would become deprecated.

Comment 11

[Keyser Sose]

Marking NEW.

Comment 12

[Julien Pierre]

Perhaps there is a fix after all that doesn't involve a new API.
In the case of PR_fprintf, we can assume we always want to write to a text file,
so the conversion is needed. So the conversion could be done in that function.
My previous patch was doing it in the another layer, which also affected other
functions such as PR_sprintf . We don't want these functions to change.

The fix would be to have PR_fprintf do an extra parsing pass on the string just
before writing it to the file descriptor. If any \n are encountered, it would
need to insert a \r . This might not be the most efficient thing to do, as the
I/O would be broken up into multiple pieces at that point (unless we make a copy
of the whole string, which I don't think is a good idea). The right thing to do
is probably to use scatter/gather if we can. I know OS/2 supports it in some way
but I don't see a call for it (there is no DosWriteV, only DosWrite).

Comment 13

[Julien Pierre]

Attachment: patch to insert \r for every \n written by PR_vfprintf to a PRFileDesc

This patch solves the problem much better as it doesn't affect intermediate
functions (PR_sprintf). It fixes the biggest part of the display problem of the
NSS command-line tools on OS/2.

Comment 14

[Julien Pierre]

This latest patch still has one flaw - if a program reads and then writes the
data back to the file, it will keep growing. There needs to be an additional
check to see if there is already a \r immediately preceding or following the \n
so that we don't add one in that case.

As far as scatter-gather in OS/2 is concerned, Warp Server for e-business added
a function called DosListIO to do this. We might be able to take advantage of
this in NSPR, but it would require runtime loading of the function, as it is not
available in older Warp 3.0 & Warp 4.0 kernels.

Comment 15

[Julien Pierre]

Attachment: better patch

Comment 16

[Julien Pierre]

Wan-Teh,

Could you please review the patch and check it in if appropriate ? Thanks.

Comment 17

[Wan-Teh Chang]

Comment on attachment 98015 [details] [diff] [review]
better patch

We should do this only if the fd represents
a console window.  Is it possible to detect
whether an fd is a console on OS/2?

Comment 18

[Julien Pierre]

There is a runtime library function in VACPP that peforms this test, but it
doesn't take a system FD. I'll look up the OS/2 API doc to find out how it might
be implemented.

However, I'm not sure if we want to do this test. If somebody is redirecting the
output, say piping it to another program, then the fd might no longer be a tty,
but the \r would still be a problem.

Comment 19

[Wan-Teh Chang]

This is to be consistent with the other platforms,
especially Windows, which also uses CRLF as line
endngs in text files.  The current implementation
is only broken for console windows.  If the output
is redirected via a pipe to another program, your
proposed change will cause that other program to
receive different input from what it would receive
if the current version of NSPR were used.

The need to maintain backward compatibility restricts
what we can do here.  This is why I think we should
only perform the translation when writing to a console.

Comment 20

[Julien Pierre]

Wan-Teh,

The only released NSPR application for OS/2 is the browser.
I don't think there is an existing dependency of any command-line OS/2
application based on NSPR. So, changing the behavior for such programs should be
OK. The main applications I'm trying to get to work correctly with this patch
indeed are the NSS command-line tools.

I can see that the change may be a problem if PR_fprintf is used to write data
files in the browser, though, as it would change its format.

Actually, I just found out tha the C runtime "isatty" function works on a native
handle, so we can use it here. I have tried it and it works fine.

Comment 21

[Julien Pierre]

Attachment: updated patch using isatty

Comment 22

[Wan-Teh Chang]

Comment on attachment 100500 [details] [diff] [review]
updated patch using isatty

Does "\r\n" also produce the correct output
in a console on OS/2?  Your patch avoids adding
an extra '\r' after "\r\n".

Comment 23

[Julien Pierre]

Yes, it does.
\r resets the cursor to column 0. So it can be done before or after a \n, and
doesn't need to be done more than once.

Comment 24

[Julien Pierre]

Moving to 4.3 , since this didn't get checked in before.

Comment 25

[Julien Pierre]

Wan-Teh,

Could you please check this into the tip of NSPR ? This patch will not affect
the Mozilla browser as isatty is used to check if we are on a terminal.

Thanks.

Comment 26

[Wan-Teh Chang]

Comment on attachment 100500 [details] [diff] [review]
updated patch using isatty

Is this patch only necessary for the VACPP runtime
library?  I am wondering if the gcc builds also need
this patch.

Comment 27

[Julien Pierre]

Wan-Teh,

In the PR_Write code path, we write directly to the native handle with a
DosWrite. So this patch is independent of the runtime library. This is why I
used #ifdef XP_OS2 rather than #ifdef XP_OS2_VACPP .

Comment 28

[Julien Pierre]

I built NSPR with gcc successfully on OS/2 today. 

[on a different subject : I could not use the tip but rather a branch,
NSPRPUB_PRE_4_2_CLIENT_BRANCH . Please check in the changes to the tip of NSPR
as soon as possible ]

I verified that this fix was still necessary with the gcc build, and worked with
it as intended.

Comment 29

[Wan-Teh Chang]

Comment on attachment 100500 [details] [diff] [review]
updated patch using isatty

>+            if ('\n' == *(msg+curofs) && /* current character is \n */
>+                ( (0 == curofs)   || ('\r' != *(msg+curofs-1)) ) && /* previous character isn't \r */
>+                ( (len == curofs) || ('\r' != *(msg+curofs+1)) )    /* next character isn't \r */

The (len == curofs) test is not needed because it must
be false if the current character is \n.  (The character
at offset 'len' is the terminating null byte.)

Agreed?

Comment 30

[Wan-Teh Chang]

Attachment: updated patch using isatty v2

I edited the patch to conform to NSPR's coding style.

The only real difference from Julien's patch is that
the useless (len == curofs) test that I pointed out
was removed.

Comment 31

[Wan-Teh Chang]

Fixed checked into the NSPR tip (4.4) and the
NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilla 1.4 beta).