libtomcrypt icon indicating copy to clipboard operation
libtomcrypt copied to clipboard

Polish API `char*` vs. `unsigned char*`

Open sjaeckel opened this issue 7 years ago • 9 comments

e.g. the Base64 API uses unsigned char* where char* should have been used.

Check all API's that work with "strings" and correct this.

sjaeckel avatar Mar 22 '18 15:03 sjaeckel

The only issue I see with char* case is the fact that some people may assume that char* means passing/returning a string with NUL byte at the end, which is not our current behaviour.

For example libsodium adds NUL byte when encoding B64/HEX - https://download.libsodium.org/doc/helpers/

karel-m avatar Mar 23 '18 11:03 karel-m

passing/returning a string with NUL byte at the end, which is not our current behaviour.

well that's only true for our decoders, the encoders already NUL-terminate the output...

sjaeckel avatar Mar 23 '18 11:03 sjaeckel

You are right base64_encode and base64url_encode do append NUL byte; however, base32_encode does not (it should at least for consistency).

Decoders are IMO not a problem.

karel-m avatar Mar 23 '18 13:03 karel-m

base32_encode does not

yeah, it should! you can add it to #353 if you want to :)

Decoders are IMO not a problem.

nope, but I'd say either we change the existing API or we add base64_decode_str() etc. (which I'd prefer as the existing API has also its valid use-cases)

sjaeckel avatar Mar 23 '18 14:03 sjaeckel

yeah, it should! you can add it to #353 if you want to :)

I think #353 seems like ready to merge, I'd fix it in a separate PR. I also plan to suggest a small change to base64_decode where the relaxed mode seems to be more relaxed than it should be.

karel-m avatar Mar 23 '18 14:03 karel-m

I also plan to suggest a small change to base64_decode where the relaxed mode seems to be more relaxed than it should be.

oh kay... now I'm curious :)

sjaeckel avatar Mar 23 '18 14:03 sjaeckel

@sjaeckel do you see any troubles with switching to use 'char*' in base64_* ?

karel-m avatar Mar 25 '18 16:03 karel-m

@sjaeckel do you see any troubles with switching to use 'char*' in base64_* ?

not really

sjaeckel avatar Mar 25 '18 16:03 sjaeckel

FYI: there are two related PR #366 and #367

I would like to propose changing:

int base16_decode(const char *in, unsigned char *out, unsigned long *outlen)

to:

int base16_decode(const char *in, unsigned long inlen, unsigned char *out, unsigned long *outlen)

It will be consistent with base64_encode and base32_encode and IMO it is also a bit more developer-friendly as you do not need to safe-check in buffer before calling base16_decode.

karel-m avatar Mar 25 '18 20:03 karel-m