@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @captain-caveman2k and @tatsuhiro-t to be potential reviewers.

Would it make sense to checker for '\033', too? That is the character that breaks our terminals most of the time. On the other hand, I am not sure if it commonly occurs without hitting NUL first...

I'm also concerned about false positives with utf-8 contents, which I suppose can have that value legitimately. I figured the zero check is fairly safe for that.

UTF-8 is constructed so that there are never any ASCII bytes (i.e. 0x00 to 0x7F) in the (valid) encodings of non-ASCII characters, so there might not be any real false positive concerns.

It might be interesting to look at how e.g. grep or less implement their similar is-binary tests.

grep: looks for binary zeroes. See the buf_has_nulls function here: https://git.savannah.gnu.org/cgit/grep.git/tree/src/grep.c

less (found no way to link to a source code online so I quote a comment in the filename.c source file): "Call it a binary file if there are more than 5 binary characters in the first 256 bytes of the file"

Decimal 27 is used for escape sequences, which is exactly why it is so annoying to get in a binary file since it can send the most crazy, illegal or weirdo sequences. But when used in a properly crafted stream, it can be used to set colors etc and then definitively (well, I think so at least) is not "binary". The http://wttr.in service is an example of a site that uses escape sequences without the response being considered "binary" by most people.

Good point. I did not realize there are sites that intentionally print escape sequences to our terminals. It would be really bad idea to handle them as binary data.

The zero check would trip up on UTF-16 and UTF-32. While I haven't ever seen UTF-32 text on a web server, UTF-16 does exist "in the wild" and can usually be displayed correctly by terminals, since they only drop the \000.

I don't think \033 is used to encode any UTF-8 characters aside from a literal \033, since all continuation bytes are >=128. A "correct" solution for all this may be to use libmagic.

We could donate about 90 lines of code we use to handle a similar problem in out product. It first identifies text with a BOM, then explicitly checks for GIF, JPEG, gzip and PDF files, since those may contain lots of text-like characters at the start, and then runs a heuristic by counting some characters < 32 and sequences of 2 or more \000. If there's interest in this code, I could mail or post that, or make a merge request (the latter may take a few days).

When we discussed this issue at the curl meet, I thought the easiest solution would be to replace all non-whitespace characters < 32, all characters 128-159, and possibly everything >=128 with a substitute, e.g. '.'.

@bagder The http://wttr.in service is an example of a site ...

Nice, but a bit too wide for the Windows console. This is a cool example too:

curl -s http://adoxa.altervista.org/ansicon/ANSI%%20Prompt%%20Colours.txt | cat

how about --printable-only :

diff --git a/src/tool_cb_wrt.c b/src/tool_cb_wrt.c
index 6c08943..08e538d 100644
--- a/src/tool_cb_wrt.c
+++ b/src/tool_cb_wrt.c
@@ -137,7 +137,17 @@ size_t tool_write_cb(char *buffer, size_t sz, size_t nmemb, void *userdata)
   if(!outs->stream && !tool_create_output_file(outs))
     return failure;
 
-  rc = fwrite(buffer, sz, nmemb, outs->stream);
+  if(config->global->printable_only) {
+    char *p, *end = buffer + (sz * nmemb);
+    for(rc = 0, p = buffer; p != end; ++p, ++rc) {
+      bool printable = isprint(*p) || *p == '\r' || *p == '\n' || *p == '\t';
+      if(fputc((printable ? *p : '.'), outs->stream) == EOF)
+        break;
+    }
+  }
+  else {
+    rc = fwrite(buffer, sz, nmemb, outs->stream);
+  }
 
   if((sz * nmemb) == rc)
     /* we added this amount of data to the output */
diff --git a/src/tool_cfgable.h b/src/tool_cfgable.h
index 38777f6..25c303a 100644
--- a/src/tool_cfgable.h
+++ b/src/tool_cfgable.h
@@ -259,6 +259,7 @@ struct GlobalConfig {
   int progressmode;               /* CURL_PROGRESS_BAR / CURL_PROGRESS_STATS */
   char *libcurl;                  /* Output libcurl code to this file name */
   bool fail_early;                /* exit on first transfer error */
+  bool printable_only;            /* terminal output '.' for non-printables */
   struct OperationConfig *first;
   struct OperationConfig *current;
   struct OperationConfig *last;   /* Always last in the struct */
diff --git a/src/tool_getparam.c b/src/tool_getparam.c
index 56bbbf1..04a171c 100644
--- a/src/tool_getparam.c
+++ b/src/tool_getparam.c
@@ -251,6 +251,7 @@ static const struct LongShort aliases[]= {
   {"E7", "proxy-capath",             ARG_STRING},
   {"E8", "proxy-insecure",           ARG_BOOL},
   {"E9", "proxy-tlsv1",              ARG_NONE},
+  {"EA", "printable-only",           ARG_BOOL},
   {"f",  "fail",                     ARG_BOOL},
   {"fa", "fail-early",               ARG_BOOL},
   {"F",  "form",                     ARG_STRING},
@@ -1559,6 +1560,11 @@ ParameterError getparameter(const char *flag, /* f or -long-flag */
         config->proxy_ssl_version = CURL_SSLVERSION_TLSv1;
         break;
 
+      case 'A':
+        /* terminal output '.' for non-printables */
+        global->printable_only = toggle;
+        break;
+
       default: /* unknown flag */
         return PARAM_OPTION_UNKNOWN;
       }

how about --printable-only

Made like that, it doesn't save anyone from accidentally sending binary to stdout, right? That's sort of the main point with my take on this. And enabling --printable-only by default if the output is detected to be a tty also seems a bit... strange, since it's a very subtle change.

Made like that, it doesn't save anyone from accidentally sending binary to stdout, right?

Right. My suggestion was something lighter, so rather than error on binary by default we don't do that. Instead of making a change to the defaults we could offer an option that the user can add to their curlrc if they don't want non-printable characters messing with their terminal, and those characters replaced with .. In the case of either on/off there wouldn't be an error, some character would be output. Binary would remain ok by default.

Note the condition I used didn't check for isatty simply because I forgot, it should be if(config->global->isatty && config->global->printable_only). Also reading through this I'm wondering about UTF-16 and UTF-32, for example if we go char by char regardless of which of our options, we have to consider whether it's going to disrupt those encodings from being properly displayed (if that's possible with curl, I haven't tried it).

If you consider this a security problem (cf. http://seclists.org/oss-sec/2017/q2/183 ), then isprint() would obviously solve it, although it is very heavy-handed. I'm not sure if filtering \033 would be sufficient, or if the corresponding 8bit control characters would have to be filtered as well (2.5.1 in http://vt100.net/docs/vt220-rm/chapter2.html), since e.g. xterm supports 8bit controls, although those appear to be off by default.

So, what's the scope of this - just prevent accidental messes or protect the user from potentially malicious content?

This pull-request is an attempt to help users avoid accidental spewing binary data to the terminal.

Requiring a special option to accomplish this doesn't help (since these users would avoid the problem all together by using a command line option already), and it is not an attempt to block "malicious content". (The malicious examples in the link above are about how terminal software is vulnerable to certain data and I really cannot see how it can be curl's job to filter its output for that purpose.)

Renamed the option to --binary-stdout and added two test cases that verify that the feature and option work as supposed.

Coverage increased (+0.007%) to 73.169% when pulling af124e0 on bagder/avoid-binary-to-terminal into efc83d6 on master.

The name --binary-stdout implies that without it stdout isn't binary. --binary-stdout sounds like something that should always open stdout in binary mode. This is just stdout terminal, what about something that has term or terminal in the name and will toggle well like --terminal-binary --no-terminal-binary --terminal-allow-binary --no-terminal-allow-binary --term-allow-binary etc. If I saw that in a curlrc I'd have a better idea what it meant. But really these are also kind of misleading along with the doc because it doesn't strictly stop binary data since it only checks the first 2000 bytes. In other words any phrasing or option I think we don't want to create a false assurance.

No option name can completely stand for itself without being ridiculously long, they do require that the user reads up on it to really get to know what it does and how it works. I personally always prefer shorter rather than longer option names. The fact that it checks the first 2000 bytes is mostly for a functional reason: getting the warning after already having shown a lot of data feels weird. I don't think that will trick any user. Maybe --binary-ok is still the best name...

--allow-binary (via) has a nice ring to it I think.

Ok, an even better suggestion IMHO:

--output - or -o -

That is, make it explicit that the output should be sent to stdout using the existing option for this purpose!

Love it!
I've seen this pattern in some other tools. Sometimes the "file" is called -, sometimes stdout.
Maybe, while on it, we could allow stdout and stderr there?

while on it, we could allow stdout and stderr there?

We already use the single letter - to mean stdout (or stdin) for a whole bunch of options. Reserving the words "stdout" and "stderr" for the same thing now I think has a too high risk of damaging people's existing scripts... (and is slightly off-topic for this particular PR)

Was just a thought.
Anyway, the -o - is then just perfect IMHO.

Now pushed the updated version here. The warning text now says:

Warning: Binary output can mess up your terminal. Use "--output -" to tell 
Warning: curl to output it to your terminal anyway, or consider "--output 
Warning: <FILE>" to save to a file.

Coverage increased (+0.04%) to 73.42% when pulling 2789e1f on bagder/avoid-binary-to-terminal into de4c747 on master.