Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Padding correctness #1

Closed
lshamis opened this issue Nov 11, 2019 · 3 comments
Closed

Padding correctness #1

lshamis opened this issue Nov 11, 2019 · 3 comments

Comments

@lshamis
Copy link

lshamis commented Nov 11, 2019

I'm not sure the output is always correct wrt the padding at the end of the generated encoding (though it is still reversible).

char kBase64Chars[] =
    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

char* out = malloc(turbob64len(64) + 1024);
memset(out, 0, turbob64len(64) + 1024);
turbob64enc(kBase64Chars, 64, out);

The output:
QUJDREVGR0hJSktMTU5PUFFSU1RVVldYWVphYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ejAxMjM0NTY3ODkrLwAAAA==
which is 92 char.

The expected output is:
QUJDREVGR0hJSktMTU5PUFFSU1RVVldYWVphYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ejAxMjM0NTY3ODkrLw==
which is 88 char.

Note also

turbob64len(64) == 88

but will cause an overrun. This seem to be a known issue, since the benchmark over-allocates by 1024, but I'm not seeing any documentation.

@powturbo
Copy link
Owner

Thanks for reporting.
I've adjusted now the boundary checking.
There is no more reading/writing beyond the buffer end.
Please, check again.

@lshamis
Copy link
Author

lshamis commented Nov 11, 2019

Thanks! I think that did it :)

@lshamis
Copy link
Author

lshamis commented Nov 11, 2019

You might want to update the benchmark to not add the 1024.

qoega added a commit to qoega/Turbo-Base64 that referenced this issue Jul 9, 2020
While running ClickHouse tests on different servers with several sanitizers we encounter this error under UndefinedSanitizer.
```
Logging trace to /var/log/clickhouse-server/clickhouse-server.log
Logging errors to /var/log/clickhouse-server/clickhouse-server.err.log
../contrib/base64/turbob64sse.c:418:25: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
    #0 0x1a70894c in cpuisa /build/obj-x86_64-linux-gnu/../contrib/base64/turbob64sse.c:418:25
    powturbo#1 0x1a708b18 in tb64ini /build/obj-x86_64-linux-gnu/../contrib/base64/turbob64sse.c:485:13
    powturbo#2 0x198e52ec in DB::registerFunctionBase64Encode(DB::FunctionFactory&) (/usr/bin/clickhouse+0x198e52ec)
    powturbo#3 0x198b6c75 in DB::registerFunctionsString(DB::FunctionFactory&) (/usr/bin/clickhouse+0x198b6c75)
    powturbo#4 0x1696f486 in DB::registerFunctions() (/usr/bin/clickhouse+0x1696f486)
    powturbo#5 0x15390b4f in DB::Server::main(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /build/obj-x86_64-linux-gnu/../programs/server/Server.cpp:220:5
    powturbo#6 0x20c4d36f in Poco::Util::Application::run() /build/obj-x86_64-linux-gnu/../contrib/poco/Util/src/Application.cpp:334:8
    powturbo#7 0x1538fd9d in DB::Server::run() /build/obj-x86_64-linux-gnu/../programs/server/Server.cpp:184:25
    powturbo#8 0x153b0eb0 in mainEntryClickHouseServer(int, char**) /build/obj-x86_64-linux-gnu/../programs/server/Server.cpp:1084:20
    powturbo#9 0x1531cdce in main /build/obj-x86_64-linux-gnu/../programs/main.cpp:324:12
    powturbo#10 0x7fd9ab5761e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)
    powturbo#11 0x152fb02d in _start (/usr/bin/clickhouse+0x152fb02d)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../contrib/base64/turbob64sse.c:418:25 in
```
I'm running UBSAN build on server with **avx512vl**
```
cpuinfo
flags    : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon rep_good nopl xtopology cpuid tsc_known_freq pni pclmulqdq vmx ssse3 fma cx16 pcid sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch cpuid_fault invpcid_single pti tpr_shadow vnmi flexpriority ept vpid fsgsbase tsc_adjust bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx avx512f avx512dq rdseed adx smap clflushopt clwb avx512cd avx512bw avx512vl xsaveopt xsavec xgetbv1 xsaves arat umip pku ospke
```

To fix signed integer overflow I propose this one char fix. `1u` has uint type and there will be no error.

If you need more context, feel free to ask me about it.

ClickHouse issue: ClickHouse/ClickHouse#12318
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

No branches or pull requests

2 participants