bugFreeciv - Bugs: bug #22282, convert_string() / iconv() error...

 
 
Show feedback again

bug #22282: convert_string() / iconv() error reporting could be more specific

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sun Jul 6 11:39:24 2014  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: NoneOpen/Closed: Closed
Release: Operating System: Any
Planned Release: 2.4.4, 2.5.0, 2.6.0Contains string changes: None

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue Sep 30 14:45:22 2014, comment #12:

I had another look at the command console output -

1. I changed the font to the unicode Lucida Console

2. I tested using CHCP 850 and then CHCP 1252

result is below.

Anonymous
Tue Sep 30 09:16:05 2014, comment #11:

My test ruleset had invalid/incomplete UTF-8 sequences which are pretty much guaranteed to generate a complaint.

> there is no message for the mangling of Eva Duarte de Perón in
> the server console

I assume what you're referring to here is that where you expect to see
U+00F3 LATIN SMALL LETTER O WITH ACUTE
you actually see
U+00BE VULGAR FRACTION THREE QUARTERS
in the server console.

Failure to complain about this does not necessarily indicate a bug in the patch; it's more likely that freeciv-server/iconv has the wrong idea about the encoding that the command-prompt window is actually using. If so, this is a separate bug (although I don't know if there's anything we can usefully do about it).

A handy character-set confusion utility suggests that the command prompt window is probably in CP850 where freeciv-server thinks it's in CP1252 or ISO-8859-1 or similar:

F3 = U+00F3 in: ISO-8859-1, ISO-8859-1 with X11 line drawing, ISO-8859-2, ISO-8859-3, ISO-8859-9, ISO-8859-10, ISO-8859-13, ISO-8859-14, ISO-8859-15, ISO-8859-16, CP1250, CP1252, CP1254, CP1257, CP1258, VISCII, DEC MCS, PDFDocEncoding
F3 = U+00BE in: CP850

Jacob Nevins <jtn>
Project Administrator
Tue Sep 30 04:37:07 2014, comment #10:

Have just compiled and am testing rev26629.
This patch is included.

In testing I see the correct error message for the mangling of Bobby Tables✠but there is no message for the mangling of Eva Duarte de Perón in the server console.

Is this correct? I notice that the client handles Perón correctly.

Anonymous
Mon Sep 29 08:19:22 2014, SVN revision 26616:

Improve error reporting for character conversion errors.

See gna bug #22282.

(Browse SVN revision 26616)

Jacob Nevins <jtn>
Project Administrator
Mon Sep 29 08:13:00 2014, SVN revision 26613:

Improve error reporting for character conversion errors.

See gna bug #22282.

(Browse SVN revision 26613)

Jacob Nevins <jtn>
Project Administrator
Mon Sep 29 08:07:02 2014, SVN revision 26610:

Improve error reporting for character conversion errors.

See gna bug #22282.

(Browse SVN revision 26610)

Jacob Nevins <jtn>
Project Administrator
Sat Sep 27 16:35:57 2014, comment #6:

OK, thanks. That's good enough for me to commit this (and then we might learn a little more about the real instances of this error).

Jacob Nevins <jtn>
Project Administrator
Sat Sep 27 14:45:41 2014, comment #5:

Apologies - I did create a version with the patch. But I tested two non-patched versions. My compiled build without the patch and the official release are the two versions that are identical.

Here is the result for the patched version.
Invalid string conversion from UTF-8 to CP1252: Illegal byte sequence.

The two screen shots from comment #3: seem to be showing a similar gtk theme error as bug #21475: Text background problems with gtk2 >=2.24.21

Anonymous
Sat Sep 27 12:39:03 2014, comment #4:

> Invalid string conversion from UTF-8 to CP1252.2: Bobby TablesÔ£ has been added as human player.
> Error after the patch is identical on my machine.

Thanks for testing!

However, I think the patch cannot have been correctly applied in your build, as it changes the only string of the form "Invalid string conversion from %s to %s." to "... %s to %s: [some message]\n".

> Do I need to adjust the way iconv is configured.

I don't know; it's clearly not quite right, but that won't be why my diagnostic has made no difference to your build.
From your screenshots it looks like iconv is probably basically working?

Jacob Nevins <jtn>
Project Administrator
Sat Sep 27 03:39:17 2014, comment #3:

Have compiled a windows build with this patch.

Tests while taking leader Bobby Tables✠from ruleset below.

Error before patch in server console - Invalid string conversion from UTF-8 to CP1252.2: Bobby TablesÔ£ has been added
as human player.

Error after the patch is identical on my machine. Do I need to adjust the way iconv is configured.
This is the compiler warning I get about iconv

In the client the attached files show how this name is displayed.
see msg11.jpg and msg12.jpg No error message is displayed.

(file #22392, file #22393)

Anonymous
Tue Sep 23 20:53:57 2014, comment #2:

We seem to have a few reports of crashes and errors in this area, so getting more diagnostics into our standard builds seems like a good idea.

Patch attached. However, I will not commit it without some evidence that it doesn't break Windows builds; if someone can make a test build of some kind containing this patch, that would be great (crosser will probably do here unless its iconv is massively different to our standard builds) -- I'm happy to do the actual test if that's easier.

Also attached is a ruleset file (for trunk) with deliberately malformed UTF-8 sequences in leader names. That certainly provokes these errors on Linux; I haven't tested our existing Windows builds' reaction yet.

(file #22361, file #22362)

Jacob Nevins <jtn>
Project Administrator
Mon Aug 25 12:39:50 2014, comment #1:

I wondered about just adding fc_strerror(fc_get_errno()) to the string.

However, on Windows, that turns into FormatMessage(GetLastError()), effectively. This isn't appropriate for errors returned by iconv, which I think sets the real errno (which is separate from GetLastError()) and thus should have the real system strerror() called on it. (I think Windows has an EILSEQ/EINVAL etc so should be able to provide useful strings for it.)

Nearby we have

which I think will currently do the Wrong Thing on Windows (should it ever occur), so should be fixed at the same time.

However, I can't test Windows builds, so I'm not confident making these changes.

(I guess we probably get away with calling fc_strerror() after fopen() etc because that probably also sets the error code retrieved from GetLastError() as well as the POSIX-y errno? But I don't think the third-party iconv library will do so, or at least I've found no evidence in the source code.)

Jacob Nevins <jtn>
Project Administrator
Sun Jul 6 11:39:24 2014, original submission:

Currently convert_string() just says "Invalid string conversion from %s to %s." if it encounters an error related to the body of the string.

There are some defined errno meanings for iconv(), so the message could sometimes be more specific.

Jacob Nevins <jtn>
Project Administrator

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #22392:  msg11.JPG added by None (62kB - image/jpeg)
file #22393:  msg12.JPG added by None (30kB - image/jpeg)
file #22361:  trunk-S2_5-S2_4-iconv-diagnostics.patch added by jtn (2kB - text/x-patch - trunk/S2_5/S2_4 r26555 + test case)
file #22362:  british.ruleset added by jtn (2kB - application/octet-stream - trunk/S2_5/S2_4 r26555 + test case)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jtn (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Error: not logged in

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon Sep 29 08:22:10 2014jtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sat Sep 27 17:06:45 2014jtnStatusNeed Info=>Ready For Test
      Planned Release=>2.4.4, 2.5.0, 2.6.0
    Sat Sep 27 03:39:17 2014NoneAttached File-=>Added msg11.JPG, #22392
      Attached File-=>Added msg12.JPG, #22393
    Tue Sep 23 20:53:57 2014jtnAttached File-=>Added trunk-S2_5-S2_4-iconv-diagnostics.patch, #22361
      Attached File-=>Added british.ruleset, #22362
      StatusNone=>Need Info
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup