Normalize EOL to CRLF for text-mode signatures.

[ni4]

Previously we didn't distinguish text-mode document signatures from binary ones, so for non-canonical line endings (i.e. non-CRLF) validation failed.
This PR is aimed to fix this misbehavior.

Fixes #1226
Closes #1228

[ronaldtse]

LGTM @ni4 !

[rrrooommmaaa]

In my case, test_stream_signatures fails because src/tests/data/test_stream_signatures/source.txt received extra CR when cloning from github without git config --global core.autocrlf false.
Shouldn't we update test files in this PR so they no longer depend on github settings?

[ni4]

Shouldn't we update test files in this PR so they no longer depend on github settings?

Actually, some signatures there are done in binary mode, so tests will actually fail if eol sequence is changed in source.txt
Text-mode signatures would (since this PR) work well once LF is changed to CRLF.

[rrrooommmaaa]

@ni4 The question is -- the test will remain dependent on git settings?

[ni4]

@rrrooommmaaa Please see the issue #1268

And, getting back to review, do you have any further comments/suggestions on this PR?

[rrrooommmaaa]

@ni4 I expect that lastcr should also be handled in singed_src_finish in case it was the last character of the buffer? And then it should be replaced with CRLF? Is that what you meant by /* we support CR, LF and CRLF line endings */?

[ni4]

I expect that lastcr should also be handled in singed_src_finish in case it was the last character of the buffer

@rrrooommmaaa Once CR character is reached, CRLF is written. I.e. the only purpose of this flag - to detect case where CR is the last character in buffer, and the possible following LF is sent in the next signed_src_update() call.

[rrrooommmaaa]

Once CR character is reached, CRLF is written

got it now. Can you add a remark in the code describing the purpose of lastcr field, please?
The problem is that the name describes how it is set, and I have to understand how it is used from the code.
Also, why isn't variable en called end? Is there a standard for this?

[ni4]

Can you add a remark in the code describing the purpose of lastcr field, please?
The problem is that the name describes how it is set, and I have to understand how it is used from the code.

Added comment and force-pushed.

Also, why isn't variable en called end? Is there a standard for this?

Just got used to this style, to use 2 chars for temporary variable name, where i,j,k are not applicable.
Maybe it is related to past Pascal experience, where end is a reserved word.

[dewyatt]

Also, why isn't variable en called end? Is there a standard for this?

Just got used to this style, to use 2 chars for temporary variable name, where i,j,k are not applicable.
Maybe it is related to past Pascal experience, where end is a reserved word.

I'd vote against this style personally. I like names to be as descriptive as possible, just not overly long.

[ni4]

I'd vote against this style personally. I like names to be as descriptive as possible, just not overly long.

Ok, np. While it is clear how to rename en, what would be better to call ch, linebg? current_char, line_begin seems to be too long. This could also lead to clang-formatting single code line to 2-3 lines, making code less readable.

[dewyatt]

I'd vote against this style personally. I like names to be as descriptive as possible, just not overly long.

Ok, np. While it is clear how to rename en, what would be better to call ch, linebg? current_char, line_begin seems to be too long. This could also lead to clang-formatting single code line to 2-3 lines, making code less readable.

I would read ch as character and I believe that it's a well-known abbreviation so I don't see a problem there.
I would read bg as background. Typically beg would be used to abbreviate begin. So personally I might use linebeg, begline, line_beg, etc., those all seem pretty readable as a native.

I may be biased specifically on en because I don't see that used as an abbreviation for enable very often (usually only in firmware/embedded/kernel code).

EDIT: ena is a slightly clearer abbreviation for enable

[ni4]

@dewyatt Thanks for the detailed explanation. Renamed/force-pushed.

[rrrooommmaaa]

@ni4 As long as lastcr is used to keep the state (last character) between update calls, I think, it's worthwhile
creating edge case tests using callback input, so that we can emulate chunks ending with 'CR' followed by 'LF' or non-LF.
Do you agree?

[ni4]

@rrrooommmaaa Please see the updated PR. Actually it is simpler then input_from_callback since we know that information for signed_src_update() is read by 32k chunks.

[rrrooommmaaa]

This piece of code will work in 99.9999% cases, I understand, still, it doesn't look clean, as in theory the buffer might be at the very end of virtual memory, so buf+len can be 0x00000 or an overflow exception can be thrown,
so ch<end and ch<end+1 may be false right away.

[ni4]

@rrrooommmaaa Thanks for pointing at this. While it should not happen in real-life systems, this may be hit on low-memory embedded systems or so on. Added a commit.

[rrrooommmaaa]

I suggest simply to replace ch < end and ch + 1 < end with !=
I know that < is generally safer than != but in this case it would be neater

[ni4]

@rrrooommmaaa That was the first idea, however then we would end up in need to check whether ch++ overflows, adding one more check on each byte processed.

[rrrooommmaaa]

Ok, merging then.

[antonsviridenko]

EDIT: ena is a slightly clearer abbreviation for enable

I vote for "enbl" :)

[rrrooommmaaa]

I suggest simply to replace ch < end and ch + 1 < end with !=
I know that < is generally safer than != but in this case it would be neater