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 06 Jul 2014 11:39:24 AM UTC  
 
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.0

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 30 Sep 2014 02:45:22 PM UTC, 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 30 Sep 2014 09:16:05 AM UTC, 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 30 Sep 2014 04:37:07 AM UTC, 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 29 Sep 2014 08:19:22 AM UTC, SVN revision 26616:

Improve error reporting for character conversion errors.

See gna bug #22282.

(Browse SVN revision 26616)

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

Improve error reporting for character conversion errors.

See gna bug #22282.

(Browse SVN revision 26613)

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

Improve error reporting for character conversion errors.

See gna bug #22282.

(Browse SVN revision 26610)

Jacob Nevins <jtn>
Project Administrator
Sat 27 Sep 2014 04:35:57 PM UTC, 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 27 Sep 2014 02:45:41 PM UTC, 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 27 Sep 2014 12:39:03 PM UTC, 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 27 Sep 2014 03:39:17 AM UTC, 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 23 Sep 2014 08:53:57 PM UTC, 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 25 Aug 2014 12:39:50 PM UTC, 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 06 Jul 2014 11:39:24 AM UTC, 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.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 29 Sep 2014 08:22:10 AM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sat 27 Sep 2014 05:06:45 PM UTCjtnStatusNeed Info=>Ready For Test
      Planned Release=>2.4.4, 2.5.0, 2.6.0
    Sat 27 Sep 2014 03:39:17 AM UTCNoneAttached File-=>Added msg11.JPG, #22392
      Attached File-=>Added msg12.JPG, #22393
    Tue 23 Sep 2014 08:53:57 PM UTCjtnAttached 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