Fixes for Encode::utf8

[pali]

Two important changes in this pull request are:

Encode::utf8: decode: Error message now contains original source sequence:

• After this change error message for invalid UTF-8 sequence when decoding will contain original invalid sequence in hex format
• Before it was partially decoded Unicode codepoint
• Fallback coderef for decode will now contains in @_ arguments whole invalid UTF-8 sequence and not only first invalid byte
• https://rt.cpan.org/Public/Bug/Display.html?id=36348

Encode::utf8: Error message in strict mode now contains "UTF-8":

• Before this patch in error message was "utf8" as name of encoding
• Now name in error message is chosen by strict mode, either "UTF-8" or "utf8"

And other two are:

Encode::utf8: Remove defines for UTF8_ALLOW_STRICT and UTF8_ALLOW_NONSTRICT

Encode::utf8: Fix applying correct UTF-8 start mask for pathological invalid sequences

@chansen @khwilliamson Please look at changes and comment or review patches.

I hope that now warning and error messages from Encode::utf8 would be more helpful.

[dankogai]

Thank you!

[dankogai]

Looks like I should VERSION++

[chansen]

The allocation of retval here would cause a memory leak if the callback invoked in call_sv throws an exception.

[pali]

Init code is same as in do_fallback_cb, I just copied it. So it needs to be fixed on both places. Any idea how to easily fix in Encode?

[chansen]

What have we learned? Duplicated code is always a bad idea? Defer the allocation until successful execution of callback?

[pali]

Duplicated code is always a bad idea?

Agree, but in this case when it is needed to call call_sv with different argument count and types we cannot do nothing more... maybe just move those perl macros like ENTER or PUSHMARK into another macros, but that would lead to even complicated code.

Defer the allocation until successful execution of callback?

Now when I'm looking at do_fallback_cb again, that function just returns new (non-mortal) SV* which is return value from perl function called via call_sv. So what is needed is to propagate mortal SV* in POPs from ENTER/LEAVE section for future processing. What is correct and safe way to do that? Calling retval = newSVsv(POPs) prior to FREETMPS?

[chansen]

retval = newSVsv(POPs); prior FREETMPS would be correct. You could also make it mortal (sv_2mortal()) prior returning the retval and remove the call to SvREFCNT_dec in the caller code, this would make the code a bit safer since it's always preferable to work with mortal SV's if possible, on a second look this would require a bit of refactoring.

[pali]

It is not a good idea to make it mortal as mortal variables are freed later, when leaving encode/decode XS function. Function do_fallback_cb can be called too many times, each time for invalid character in input string. And input string is lot of times untrusted and can comes from attacker.

I tried it with mortal variables and following code
perl -MEncode -e 'my $b = "\x{ff}" x 10000000; decode("UTF-8", $b, sub { "$_[0]" })'
eats 825MB. Without mortal variables only 58MB. Which is a huge difference.

I looked at other XS code available in perl repository and it uses following pattern:

retval = POPs;
SvREFCNT_inc(retval);
...
FREETMPS;

Looking into perl source code, FREETMPS just remove tmp flag and decrements refcount of all variables placed in tmp stack and let stack empty. So SvREFCNT_inc prevents destroying retval in FREETMPS and make it as normal non-mortal variable. Looks like it could save some memory as new variable is not needed to allocate.

But reusing SV from POPs or creating new variable via newSVsv() is not enough. That Encode code expects that returned SV has valid PV slot. So this is reason why such construct newSVpvn("",0); ... sv_catsv(retval, POPs); is used.

I will try to cleanup this code and will use above pattern.

[chansen]

Avoid magic numbers if possible. Why not use esc[UTF8_MAXBYTES * 6 + 1]?

[pali]

Looks like UTF8_MAXBYTES is not available in older perl versions (which are supported by Encode). But I can add needed #define.

[chansen]

Thats a good idea, declaring char esc[(sizeof("\\xHH") - 1) * UTF8_MAXBYTES + 1]; would make it very clear what the buffer is intended for.

[pali]

esc is not used only for \xHH escaping. It is used also for &#%u; or &#x%02X;. So putting there sizeof("\\xHH")-1 would lead to buffer overflow (those XML needs 6 bytes, not just 4) and putting sizeof("&#xHH;")-1 would be misleading that only for XML it is used. I would rather use UTF8_MAXLEN * 6 + 1 (UTF8_MAXLEN is available also in old perls)

[chansen]

@pali, sorry for the late review! I have skimmed your code and noted some issues standing out, commented in reviewing.

Besides that, good work @pali, I'm impressed with your efforts in Encode and DBD::mysql!

@pali++

--
chansen

[pali]

@chansen Thanks for review, fixes are there: #98