bug#21883: unnecessary bit shifting range limits

[Stefan Israelsson Tampe]

i think you got it. sorry for the fuzz.


Den 15 okt 2018 12:19 AM skrev "Mark H Weaver" :

> Stefan Israelsson Tampe  writes:
> > how would this slow down the code. just add the correction where you
> > throw the exception which should be in a branch outside the hot path.
>
> If you have a suggestion that's simpler than what I did in commits
> 011aec7e, 9448a078, and 1990aa91, and just as fast in the common cases,
> feel free to propose a patch.  The words above are insufficient.
>
>   Mark
>

bug#33044: Invalid read access of chars of wide string in scm_seed_to_random_state

[Tom de Vries]

Hi,

Consider min.c:
...
#include 
#include "libguile.h"

static void *
foo (void *data)
{
  return NULL;
}

int
main (void)
{
  const char *msg = setlocale (LC_CTYPE, "ja_JP.sjis");
  printf ("msg: %s\n", msg);
  scm_with_guile (foo, NULL);
  return 0;
}
...

Compiled with guile-2.2.4:
...
$ gcc min.c -I /home/vries/guile/tarball/guile-2.2.4 -lguile-2.2 -L
/home/vries/guile/tarball/guile-2.2.4/libguile/.libs
-Wl,-rpath=/home/vries/guile/tarball/guile-2.2.4/libguile/.libs -g
...

We run into a segfault:
...
$ ./a.out
msg: ja_JP.sjis
Segmentation fault (core dumped)
...

The backtrace as reported by gdb is:
...
#0  0x77b649ba in scm_variable_ref (var=0x0) at variable.c:92
#1  0x77b63868 in scm_throw (key=key@entry=0x7a9580,
args=0x7b94c0) at throw.c:266
#2  0x77b63e15 in scm_ithrow (key=key@entry=0x7a9580,
args=, no_return=no_return@entry=1)
at throw.c:611
#3  0x77af51a5 in scm_error_scm (key=key@entry=0x7a9580,
subr=,
message=message@entry=0x7ba8e0, args=args@entry=0x7b9500,
data=data@entry=0x4) at error.c:94
#4  0x77af525f in scm_error (key=0x7a9580, subr=subr@entry=0x0,
message=message@entry=0x77b93358 "Invalid read access of chars
of wide string: ~s", args=0x7b9500,
rest=rest@entry=0x4) at error.c:59
#5  0x77af5642 in scm_misc_error (subr=subr@entry=0x0,
message=message@entry=0x77b93358 "Invalid read access of chars
of wide string: ~s", args=)
at error.c:299
#6  0x77b5aa9a in scm_i_string_chars (str=,
str@entry=0x7ba900) at strings.c:571
#7  0x77b3cef8 in scm_seed_to_random_state (seed=0x7ba900) at
random.c:444
#8  0x77b3ddaa in scm_init_random () at ../libguile/random.x:3
#9  0x77b0eb41 in scm_i_init_guile (base=) at
init.c:451
#10 0x77b62128 in scm_i_init_thread_for_guile
(base=0x7fffdb10, dynamic_state=0x0) at threads.c:586
#11 0x77b62159 in with_guile (base=0x7fffdb10,
data=0x7fffdb40) at threads.c:654
#12 0x773a84a5 in GC_call_with_stack_base () from
/usr/lib64/libgc.so.1
#13 0x77b624a8 in scm_i_with_guile (dynamic_state=, data=,
func=) at threads.c:704
#14 scm_with_guile (func=, data=) at
threads.c:710
#15 0x00400786 in main () at min.c:15
...

We see that the backtrace happens while handling an "Invalid read access
of chars of wide string: ~s" error here:
...
const char *
scm_i_string_chars (SCM str)
{
  SCM buf;
  size_t start;
  get_str_buf_start (&str, &buf, &start);
  if (scm_i_is_narrow_string (str))
return (const char *) STRINGBUF_CHARS (buf) + start;
  else
scm_misc_error (NULL, "Invalid read access of chars of wide string: ~s",
scm_list_1 (str));
  return NULL;
}
...

What triggers the error is that here, we create a non-narrow string
using scm_from_locale_string:
...
#8  0x77b3ddaa in scm_init_random () at ../libguile/random.x:3
3   scm_var_random_state = scm_c_define ("*random-state*",
scm_seed_to_random_state (scm_from_locale_string
("URL:http://stat.fsu.edu/~geo/diehard.html";)));;
...

but then in scm_seed_to_random_state handle it like a narrow string by
calling scm_i_string_chars:
...
#define FUNC_NAME s_scm_seed_to_random_state
{
  SCM res;
  if (SCM_NUMBERP (seed))
seed = scm_number_to_string (seed, SCM_UNDEFINED);
  SCM_VALIDATE_STRING (1, seed);
  res = make_rstate (scm_c_make_rstate (scm_i_string_chars (seed),
scm_i_string_length (seed)));
  scm_remember_upto_here_1 (seed);
  return res;

}
...

Thanks,
- Tom

bug#33044: Reproduced using guile binary

[Tom de Vries]

Hi,

Using a simple scheme hello world:
...
$ cat hello.scm
(display "hello world")
(newline)
...
we're able to reproduce the problem using the guile binary:

$ LC_CTYPE=ja_JP.sjis /home/vries/guile/2.2/install/bin/guile -s hello.scm
Segmentation fault (core dumped)
...

[ Note: When using 2.0, we need to set GUILE_INSTALL_LOCALE=1 in the
environment, otherwise the 'LC_CTYPE=ja_JP.sjis' setting has no effect. ]

Thanks,
- Tom

bug#33044: Analysis and proposed patch

[Tom de Vries]

Hi,

I think there are two independent problems here.

---

1.

scm_seed_to_random_state should be able to handle the case that the seed
argument is a non-narrow string.

2.

The *random-state* variable is documented like this:
...
Note that the initial value of *random-state* is the same every time
Guile starts up. Therefore, if you don’t pass a state parameter to the
above procedures, and you don’t set *random-state* to
(seed->random-state your-seed), where your-seed is something that isn’t
the same every time, you’ll get the same sequence of “random” numbers on
every run.
...

However, using scm_from_locale_string to initialize *random-state* makes
it possible that *random-state* differs depending on the locale used
when starting Guile. So, we should use a string that's independent of
the locale settings.

---

The second problem is fixed by using scm_from_latin1_string instead of
scm_from_locale_string:
...
diff --git a/libguile/random.c b/libguile/random.c
index 4051d1f..6f014e1 100644
--- a/libguile/random.c
+++ b/libguile/random.c
@@ -374,7 +374,7 @@ make_rstate (scm_t_rstate *state)
  * Scheme level interface.
  */

-SCM_GLOBAL_VARIABLE_INIT (scm_var_random_state, "*random-state*",
scm_seed_to_random_state (scm_from_locale_string
("URL:http://stat.fsu.edu/~geo/diehard.html";)));
+SCM_GLOBAL_VARIABLE_INIT (scm_var_random_state, "*random-state*",
scm_seed_to_random_state (scm_from_latin1_string
("URL:http://stat.fsu.edu/~geo/diehard.html";)));

 SCM_DEFINE (scm_random, "random", 1, 1, 0,
 (SCM n, SCM state),
...

Tested on 2.0.14 and 2.2.4 tarballs.

Thanks,
- Tom

bug#33053: scm_i_mirror_backslashes assumes ASCII-compatible locale encoding

[Mark H Weaver]

The 'scm_i_mirror_backslashes' in load.c operates on C strings in the
locale encoding, and assumes that the locale encoding is ASCII
compatible.  In the Shift_JIS encoding, used in the "JP_jp.sjis" locale,
backslash '\' is mapped to a multibyte character, and the Yen sign '¥'
is represented using code 0x5C, the same code as backslash '\' in ASCII.

As a result, users of the "JP_jp.sjis" locale will have Yen signs '¥' in
their file names converted into slashes by this function.

 Mark

bug#33053: scm_i_mirror_backslashes assumes ASCII-compatible locale encoding

[Mark H Weaver]

Mark H Weaver  writes:

> The 'scm_i_mirror_backslashes' in load.c operates on C strings in the
> locale encoding, and assumes that the locale encoding is ASCII
> compatible.  In the Shift_JIS encoding, used in the "JP_jp.sjis" locale,
> backslash '\' is mapped to a multibyte character, and the Yen sign '¥'
> is represented using code 0x5C, the same code as backslash '\' in ASCII.
>
> As a result, users of the "JP_jp.sjis" locale will have Yen signs '¥' in
> their file names converted into slashes by this function.

I miswrote the locale name above.  The locale name is "ja_JP.sjis".

  Mark

bug#33057: Guile's reader assumes the port encoding is ASCII-compatible

[Mark H Weaver]

In several places, Guile's reader assumes that the port encoding is
ASCII-compatible.  For example:

* 'scm_token' reads raw bytes and passes them to the CHAR_IS_DELIMITER
  macro to check for delimiters.

* 'scm_read_mixed_case_symbol' checks for the (optional) postfix keyword
  syntax by comparing the final _byte_ read by 'read_token' with ':'.

* 'scm_read_character' uses 'read_token' to read raw bytes up to the
  next delimiter, and if it returns a single byte with value 0-127, it
  is interpreted as the associated ASCII character regardless of the
  encoding.

* 'scm_read_semicolon_comment' reads bytes until it finds one equal to
  the ASCII code for '\n'.

There may be other problems as well.

   Mark

bug#33044: Guile misbehaves in the "ja_JP.sjis" locale

[Mark H Weaver]

retitle 33044 Guile misbehaves in the "ja_JP.sjis" locale
thanks

Hi Tom,

Thanks for the report, analysis and patch.  I agree with your analysis,
and the patch looks good.

However, there's also a much deeper problem here.  You found and fixed
one occurrence of Guile assuming that the locale encoding is ASCII-
compatible.  In fact, this assumption is widespread in Guile, and I
would guess that it's widespread throughout the POSIX world.

I admit that before I saw your message, I believed that it was
legitimate to assume that the locale encoding was ASCII-compatible.  Now
I'm unsure, although I'll note that according to the 'localedef' utility
from GNU libc, this locale is "not ISO C compliant".  It printed the
following message when I asked it to generate the "ja_JP.sjis" locale:

  [warning] character map `SHIFT_JIS' is not ASCII compatible, locale not ISO C 
compliant [--no-warnings=ascii]

Shift_JIS is _mostly_ ASCII-compatible, except that code points 0x5C and
0x7E, which represent backslash (\) and tilde (~) in ASCII, are mapped
to the Yen sign (¥) and overline (‾) in Shift_JIS.  Backslash (\) and
tilde (~) are multibyte characters in Shift_JIS.

One common problem is that Guile often uses 'scm_from_locale_string' to
create Scheme strings from ASCII-only C string literals.  These should
all be changed to use either 'scm_from_latin1_string' or
'scm_from_utf8_string'.  I prefer the latter because modern C compilers
typically use UTF-8 as the default execution character set, i.e. the
character set used to encode string and character constants, regardless
of the locale settings.  GCC uses UTF-8 by default unless
-fexec-charset=CHARSET is given at compile time.  I'd prefer to promote
writing code that works for arbitrary string literals, so that code
needn't be adjusted if non-ASCII characters are later added.

A related set of problems is that Guile often applies
'scm_from_locale_string' to char* arguments passed in from the user, or
produced by third-party libraries.  These issues are more difficult to
address.  We provide several C APIs that accept C strings without
specifying what encoding is expected.  If the string ultimately derives
from a C string constant, we probably want UTF-8, whereas if the string
came from I/O, or program arguments, then we probably want the locale
encoding.

For example, consider 'scm_c_eval_string'.  This has been a public API
function since 2002, but we did not specify the encoding of its C string
argument until 2011.  We chose the locale encoding in this case, which I
think is reasonable, but I also expect that code exists in the wild that
passes a C string literal to 'scm_c_eval_string'.

Until now, problems like this have been mostly harmless, since the C
string literals are typically ASCII-only.  However, if we wish to
support non-ASCII-compatible encodings such as Shift_JIS, we can no
longer consider these problems harmless.  For example, programs which
pass C string literals to 'scm_c_eval_string' will fail when using the
"ja_JP.sjis" locale, if any tildes or backslashes are present.
Backslashes are fairly common in Scheme code.

There's various other code scattered in Guile that assumes ASCII
characters can searched for, and sometimes replaced with other ASCII
characters.  For example, several functions in load.c, including
'search_path', 'load_thunk_from_path' scan through file names in the
locale encoding, scanning the bytes looking for particular ASCII codes
such as '.', '/', and '\'.

On MingW, 'scm_i_mirror_backslashes' in load.c converts backslashes into
forward slashes byte-wise, assuming ASCII-compatibility, and this
transformation is applied to file names in several places.

While looking into this, I also discovered that Guile's S-expression
reader, i.e. the 'read' procedure, assumes an ASCII-compatible port
encoding, despite the fact that it is meant to support arbitrary
encodings such as UTF-16 and UTF-32.  I just filed a related bug
 to track this probem.

These are some of the problems that I'm currently aware of.  I expect
that this bug report will remain open for a while.

To begin, I've started working on a patch to change many occurrences of
'scm_from_locale_string' to 'scm_from_utf8_string', in cases where the C
string clearly originates from a C string literal.

Thanks again for the detailed bug report and analysis.

Regards,
  Mark

bug#33044: Guile misbehaves in the "ja_JP.sjis" locale

[Mark H Weaver]

Mark H Weaver  writes:

> Shift_JIS is _mostly_ ASCII-compatible, except that code points 0x5C and
> 0x7E, which represent backslash (\) and tilde (~) in ASCII, are mapped
> to the Yen sign (¥) and overline (‾) in Shift_JIS.  Backslash (\) and
> tilde (~) are multibyte characters in Shift_JIS.

Although I wrote above that "Backslash (\) and tilde (~) are multibyte
characters in Shift_JIS", that was admittedly my assumption, based on
the absence of those characters in the "First byte" map shown here:

  https://en.wikipedia.org/wiki/Shift_JIS#As_defined_in_JIS_X_0208:1997

However, now I'm unsure.  I've spent some time attempting to find the
Shift_JIS encodings for backslash and tilde, but I've not yet found an
answer.

I've asked Emacs 26 to write a file containing backslashes and Yen signs
using the "shift_jis" encoding, and both characters seem to be mapped to
the same code: 0x5C.

I've also used the 'iconv' utility from GNU libc to convert backslashes
and Yen signs to Shift_JIS, and it also maps these two characters to the
same codes:

--8<---cut here---start->8---
mhw@jojen ~$ echo '\\¥¥' | iconv -f UTF-8 -t SHIFT-JIS > Shift_JIS_test.txt
mhw@jojen ~$ hexdump -C Shift_JIS_test.txt
  5c 5c 5c 5c 0a|.|
0005
--8<---cut here---end--->8---

While investigating, I found this bug for GNU libc asking to add an SJIS
locale, and the developers were strongly opposed:

  https://bugzilla.redhat.com/show_bug.cgi?id=136290

At this point, I'm inclined to believe that Shift_JIS is not suitable as
a locale encoding on POSIX systems, and that we should not try to
support it in Guile.

What do you think?

Can you tell me how backslash and tilde are represented in Shift JIS?

 Regards,
   Mark