Skip to content

LVTextFileBase: fix parsing of multibytes encodings#515

Merged
poire-z merged 4 commits into
koreader:masterfrom
poire-z:fix_variable_width_encodings
Mar 23, 2023
Merged

LVTextFileBase: fix parsing of multibytes encodings#515
poire-z merged 4 commits into
koreader:masterfrom
poire-z:fix_variable_width_encodings

Conversation

@poire-z

@poire-z poire-z commented Mar 19, 2023

Copy link
Copy Markdown
Contributor

Some bytes/chars could be lost if, while reading, our buffer would end inside a multibyte character, possibly losing sync and causing garbled data.
See issue and discussion in koreader/koreader#10218.


This change is Reviewable

Some bytes/chars could be lost if, while reading, our
buffer would end inside a multibyte character, possibly
losing sync and causing garbled data.

@Frenzie Frenzie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait to put this in?

@poire-z

poire-z commented Mar 19, 2023

Copy link
Copy Markdown
Contributor Author

Wait to put this in?

No, no urgency, it's been like that for years, east asians using old encodings can wait :)

Comment thread crengine/include/lvxml.h
bool m_eof;

void checkEof(lvsize_t bytes_needed);
void checkEof(int bytes_needed);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my proper-data-type loving heart, it bleeds ;p.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried :) but as we now substracts from it, and have a size_t possibly getting a chance to become a offset_t, we'd need another variable for that different type, and it makes it noisier and harder to grasp.
(But good to be there to remind you you have a working heart :)

@NiLuJe NiLuJe Mar 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was hoping the switch was just because you started running into signedness issues, thanks ;).

Comment thread crengine/src/lvxml.cpp
Comment on lines +428 to +439
else if ( (ch & 0xE0) == 0xC0 ) { // Should be a 2-bytes wide char
checkEof(2);
}
else if ( (ch & 0xF0) == 0xE0 ) { // Should be a 3-bytes wide char
checkEof(3);
}
else if ( (ch & 0xF8) == 0xF0 ) { // Should be a 4-bytes wide char
checkEof(4);
}
else { // Should not happen?
checkEof(4); // be sure we don't get stuck
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the answer is going to be "because it matches existing code" but I'll try anyway ;p.

Could be simplified as ch < 0xE0 for 2 bytes, ch < 0xF0 for 3 bytes, and a single else for 4 bytes ;).

(c.f., https://github.com/NiLuJe/FBInk/blob/6572b5f393f810785e382104391d177120da46f6/cutef8/utf8.c#L789-L807)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it matches existing code !
Nice try anyway :)

@himisawww

Copy link
Copy Markdown

I reviewed 4 commits and now they seem perfect.

@poire-z

poire-z commented Mar 20, 2023

Copy link
Copy Markdown
Contributor Author

Thanks !
(You could have reviewed the whole resulting thing by clicking on "Files changes" at top, https://github.com/koreader/crengine/pull/515/files)

@poire-z poire-z merged commit 63ca294 into koreader:master Mar 23, 2023
@poire-z poire-z deleted the fix_variable_width_encodings branch March 23, 2023 13:08
@poire-z poire-z changed the title LVTextFileBase: fix parsing of variable width encodings LVTextFileBase: fix parsing of multibytes encodings Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants