search

OpenPrinting / cups

OpenPrinting CUPS Sources
https://openprinting.github.io/cups
Apache License 2.0
1.02k stars 182 forks source link

Regression: Certificate data is corrupted during base64 conversion #726

Closed RSilicon closed 1 year ago

RSilicon commented 1 year ago

The bug during which certificates become corrupt was introduced in a521b235a1abc008cb0b2f490a765bb31e2ec14b. It turns out some data was not being truncated after being promoted to an integer, causing 1s to be ORed into the index number when they should not have been.

I only intended to remove the & 255 from the other side, where the & 63 would have rendered that operation redundant.

I apologize for this error. I made the appropriate changes I intended to make in this new PR.

Please merge and make a new release if possible.

RSilicon commented 1 year ago

@zdohnal @tillkamppeter @michaelrsweet I apologize for this mistake.

It is important that we address this and ship the fix.

tillkamppeter commented 1 year ago

@AtariDreams as site owner I can override the need for a second person to review at any time and accept either this PR or my PR #724. Which one should I accept now? Which is the correct one? I know that #724 works for me, and this one I did not test yet. You told at first that I should revert your original change (which is #724) and now you only want a partial reversion, re-introducing the & 255 in only one line. What is correct now?

RSilicon commented 1 year ago

@AtariDreams as site owner I can override the need for a second person to review at any time and accept either this PR or my PR #724. Which one should I accept now? Which is the correct one? I know that #724 works for me, and this one I did not test yet. You told at first that I should revert your original change (which is #724) and now you only want a partial reversion, re-introducing the & 255 in only one line. What is correct now?

I found the error I made months ago. I only intended to remove the & 255 from the other side, where the & 63 would have rendered that operation redundant.

RSilicon commented 1 year ago

@AtariDreams as site owner I can override the need for a second person to review at any time and accept either this PR or my PR #724. Which one should I accept now? Which is the correct one? I know that #724 works for me, and this one I did not test yet. You told at first that I should revert your original change (which is #724) and now you only want a partial reversion, re-introducing the & 255 in only one line. What is correct now?

My change should be the correct one.

Only because I removed the & 255 from a place where I did not mean to.

RSilicon commented 1 year ago

We need to make a hotfix 2.4.5 out of this.

I feel like this is too important an issue to wait until next release.