Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#457 closed defect (fixed)

Specify encoding in all getBytes() calls

Reported by: zzz Owned by: z3r0fox
Priority: maintenance Milestone: 0.9.24
Component: api/data Version: 0.8.4
Keywords: easy Cc:
Parent Tickets: Sensitive: no

Description

To prevent errors in the presence of non-ASCII-compatible platform encodings. See ticket #436 for details. A quick grep for getBytes() yields 272 lines.

Subtickets

Attachments (7)

20151207localecorediff.md (30.4 KB) - added by z3r0fox 4 years ago.
20151207core_fixlocale.diff (30.4 KB) - added by z3r0fox 4 years ago.
Replaces instances of getBytes() in core classes
20151210apps_fixlocale.diff (57.3 KB) - added by z3r0fox 4 years ago.
Replaces instances of getBytes() in apps classes
20151213router_fixlocale.diff (18.3 KB) - added by z3r0fox 4 years ago.
Replaces instances of getBytes() in router classes
20151219core_fixlocale.diff (30.4 KB) - added by z3r0fox 4 years ago.
20151219apps_fixlocale.diff (39.9 KB) - added by z3r0fox 4 years ago.
20151219router_fixlocale.diff (6.1 KB) - added by z3r0fox 4 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by zzz

Note that the little-used utility function DataHelper?.getUTF8() exists and is probably the best replacement for String.getBytes().

comment:2 Changed 8 years ago by zzz

Milestone: 0.90.9.3
Priority: minormaintenance

comment:3 Changed 7 years ago by str4d

Milestone: 0.9.3

comment:4 Changed 6 years ago by zzz

Keywords: easy grunt work added

Findbugs now finds these ("internationalization" category) and currently lists 151 issues.

Note that we now have a DataHelper?.getASCII() method which is more efficient than getUTF8() if you know the input is 7-bit.

comment:5 Changed 5 years ago by zzz

Keywords: grunt work removed

i2ptunnel client-side in 2ce39645bf3b3b7fc2f083ebcb073f104114e939 0.9.18-12-rc
much much more to do

comment:6 Changed 4 years ago by z3r0fox

Owner: set to z3r0fox
Status: newaccepted

comment:7 Changed 4 years ago by zzz

Saw some discussion in IRC a few days ago. Changes may be too complex to be automated by e.g. sed. If you do split up the changes into separate patches for review, the best way is to split by subsystem, i.e. core, router, and each of the apps/* applications.

Changed 4 years ago by z3r0fox

Attachment: 20151207localecorediff.md added

comment:8 Changed 4 years ago by z3r0fox

Here's a first crack at the core patch. Where I could see a String class being fed into getBytes, replaced with DataHelper?.getASCII(). Otherwise, getUTF8(). Did test classes as well on the rationale they should match the system locale being used by core classes. Otherwise tried to keep diff minimal as discussed. Let me know, thanks!

comment:9 Changed 4 years ago by z3r0fox

Keywords: review-needed added

comment:10 Changed 4 years ago by zzz

ACK

I got a little trouble with Base32 encode and decode.

Decode is fine I guess to use ASCII, if there's any high bits in there it will bomb out later. Not so important that we catch every error but I think this will do it.

For encode, we don't document in the javadocs how we're taking the string, not that anybody's sending non-ASCII characters to this thing. Either ASCII or UTF-8 is ok I guess, you picked ASCII, don't know which I would have done. But we should probably clarify the javadoc for it one way or the other.

I only skimmed the test changes, which was most of it.

comment:11 Changed 4 years ago by z3r0fox

[SYN] That's fine, I can change those. Yeah there are some tricky calls. I'm attempting (not always guaranteed at 3 in the morning, so thanks for the review) to go with getASCII where there's an obvious string literal, or where I can chase the code backwards and verify some questionable parameter turns out to be an easy conversion like an int. I'm going with getUTF8 where there's user input for eg; filename, password etc because of the possibility it's in a different language. Here's the apps diff…

Changed 4 years ago by z3r0fox

Attachment: 20151207core_fixlocale.diff added

Replaces instances of getBytes() in core classes

Changed 4 years ago by z3r0fox

Attachment: 20151210apps_fixlocale.diff added

Replaces instances of getBytes() in apps classes

Changed 4 years ago by z3r0fox

Replaces instances of getBytes() in router classes

comment:12 Changed 4 years ago by zzz

All ACKS contingent on your successful testing (i.e. compiling and running the unit tests)

20151207core_fixlocale.diff: ACK

20151213router_fixlocale.diff: Please don't change anything in org/cybergarage. It's not our code, it's a mess already, changes make merging future versions harder, and I don't want to spent the time to figure out the code enough to review your changes. And it would be very difficult to test. UPnP charset is probably set in the XML or HTTP headers and can't be assumed to be UTF-8? Don't know and don't want to mess with. ACK the non-cybergarage stuff.

20151210apps_fixlocale.diff: You've changed a lot of SAM stuff (not talking about tests, but the main code) which I already said was clean. You're changing getBytes(charset) calls which is not necessary (the whole point is to specify the charset). Changing getBytes(charset) to DataHelper? is extra pointless and making things less efficient. Not only that, you're changing some ascii to utf and vice versa. Some are fine but some are mistakes. But no matter. Why are you changing getBytes(charset) calls at all. Don't. ACK the non-SAM and SAM test changes.

comment:13 Changed 4 years ago by z3r0fox

Rolled my repo back the hard way (is there an easy way?) and edited each diff, patched, committed. New diffs posted here with date 20151219_xxx. Failing two unit tests, looking into it. Thanks. Error message follows if anyone has any insight.


[junit] Test net.i2p.client.naming.BlockfileNamingServiceTest? FAILED
[junit] Test net.i2p.util.LogSettingsTest? FAILED

BUILD FAILED
/home/me/code/monotone/i2p/i2p.i2p/build.xml:1629: The following error occurred while executing this line:
/home/me/code/monotone/i2p/i2p.i2p/apps/ministreaming/java/build.xml:164: Compile failed; see the compiler error output for details.

Changed 4 years ago by z3r0fox

Attachment: 20151219core_fixlocale.diff added

Changed 4 years ago by z3r0fox

Attachment: 20151219apps_fixlocale.diff added

Changed 4 years ago by z3r0fox

comment:14 Changed 4 years ago by zzz

Keywords: review-needed removed

You didn't say what the "hard way" is, but the easy way depends on whether you'd checked things in or not. mtn questions are best asked and answered on IRC.

The Blockfile test is prone to failure. Maybe the Log test also. I wouldn't worry about it. The ministreaming test is something new added by str4d and requires dependencies I haven't figured out yet.

ACK all 3.

comment:15 Changed 4 years ago by z3r0fox

Resolution: fixed
Status: acceptedclosed

Pushed, closed.

comment:16 Changed 4 years ago by zzz

Milestone: 0.9.24

will be 0.9.23-17

Note: See TracTickets for help on using tickets.