A typo in a constant in cp932 codec

[izbyshev]

BPO	35194
Nosy	@vstinner, @methane, @serhiy-storchaka, @izbyshev, @miss-islington
PRs	bpo-35194: Fix a wrong constant in cp932 codec #10420 [3.7] bpo-35194: Fix a wrong constant in cp932 codec (GH-10420) #10423 [3.6] bpo-35194: Fix a wrong constant in cp932 codec (GH-10420) #10424 bpo-35194: cjkcodec: check the encoded value is not truncated #10432 [2.7] bpo-35194: Fix a wrong constant in cp932 codec (GH-10420) #10433

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-03-29.07:48:59.564>
created_at = <Date 2018-11-08.22:55:24.583>
labels = ['extension-modules', '3.8', 'type-bug', '3.7']
title = 'A typo in a constant in cp932 codec'
updated_at = <Date 2019-03-29.07:49:13.054>
user = 'https://github.com/izbyshev'

bugs.python.org fields:

activity = <Date 2019-03-29.07:49:13.054>
actor = 'methane'
assignee = 'none'
closed = True
closed_date = <Date 2019-03-29.07:48:59.564>
closer = 'methane'
components = ['Extension Modules']
creation = <Date 2018-11-08.22:55:24.583>
creator = 'izbyshev'
dependencies = []
files = []
hgrepos = []
issue_num = 35194
keywords = ['patch']
message_count = 13.0
messages = ['329489', '329500', '329501', '329503', '329504', '329508', '329514', '329515', '329516', '329520', '329521', '329595', '339100']
nosy_count = 5.0
nosy_names = ['vstinner', 'methane', 'serhiy.storchaka', 'izbyshev', 'miss-islington']
pr_nums = ['10420', '10423', '10424', '10432', '10433']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35194'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

[izbyshev]

UBSan with -fsanitize=implicit-integer-truncation found a suspicious one:
/scratch2/izbyshev/cpython/Modules/cjkcodecs/_codecs_jp.c:43:17: runtime error: implicit conversion from type 'unsigned int' of value 4294966013 (32-bit, unsigned) to type 'unsigned char' changed the value to 253 (8-bit, unsigned)

Indeed, the wrong constant was used (the correct one is used in corresponding decoder code at

cpython/Modules/cjkcodecs/_codecs_jp.c

Line 105 in fd512d7

OUTCHAR(0xf8f1 - 0xfd + c);

). In this case the truncation was harmless because only the lowest byte of the wrong result was used, and it was correct. But it probably makes sense to fix it if only to reduce noise from UBSan.

All Python versions are affected, but I've marked 3.8 only since I'm not sure what the policy for backporting such changes is.

[miss-islington]

New changeset 7a69cf4 by Miss Islington (bot) (Alexey Izbyshev) in branch 'master':
bpo-35194: Fix a wrong constant in cp932 codec (GH-10420)
7a69cf4

[serhiy-storchaka]

Maybe add asserts in OUTBYTE1() and similar macros to prevent similar errors in future?

[miss-islington]

New changeset 49ee41f by Miss Islington (bot) in branch '3.6':
bpo-35194: Fix a wrong constant in cp932 codec (GH-10420)
49ee41f

[miss-islington]

New changeset 22234f1 by Miss Islington (bot) in branch '3.7':
bpo-35194: Fix a wrong constant in cp932 codec (GH-10420)
22234f1

[vstinner]

Maybe add asserts in OUTBYTE1() and similar macros to prevent similar errors in future?

I like the idea. Make sure that: 0 <= ch <= 255?

[izbyshev]

I've added 'assert' to macros. Since 'typeof' seems to be disallowed in Python, I've used 'unsigned int' as the type of an intermediate variable.
Another alternative is 'assert(0 <= (c) && (c) <= 255)', but 'c' will be evaluated several times.

[serhiy-storchaka]

Converting to unsigned int can unnecessary widen the value or truncate higher bits. On other side, testing "0 <= (c)" can emit a compiler warning if c is unsigned.

Maybe test "Py_CHARMASK(c) == (c)"?

[izbyshev]

Maybe test "Py_CHARMASK(c) == (c)"?

This is a good alternative if multiple evaluation of 'c' is acceptable. Though I'd prefer '(unsigned char)c == c' for this style of fixing because it is bit closer to what happens in '((*outbuf)[i]) = c'.

[izbyshev]

I've checked than other macros in Modules/cjkcodecs/cjkcodecs.h don't avoid multiple argument evaluation (e.g. OUTCHAR2, _TRYMAP_ENC), so I've changed 'assert' to a variant of what Serhiy suggested.

[izbyshev]

OUTCHAR2 is a wrong example. Other examples are NEXT_IN, NEXT_OUT.