gh-libtom/libtomcrypt

Watch

Fork 0

Polish API `char` vs. `unsigned char` #361

New issue

Open

opened 2018-03-22 10:00:10 -05:00 by sjaeckel · 9 comments

sjaeckel commented

2018-03-22 10:00:10 -05:00

(Migrated from github.com)

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.

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.

karel-m commented

2018-03-23 06:05:51 -05:00

(Migrated from github.com)

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/

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/

sjaeckel commented

2018-03-23 06:32:21 -05:00

(Migrated from github.com)

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...

> 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...

karel-m commented

2018-03-23 08:57:51 -05:00

(Migrated from github.com)

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.

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.

sjaeckel commented

2018-03-23 09:05:59 -05:00

(Migrated from github.com)

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)

> 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)

karel-m commented

2018-03-23 09:14:28 -05:00

(Migrated from github.com)

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.

> 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.

sjaeckel commented

2018-03-23 09:20:44 -05:00

(Migrated from github.com)

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 :)

> 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 :)

karel-m commented

2018-03-25 11:20:16 -05:00

(Migrated from github.com)

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

sjaeckel commented

2018-03-25 11:33:15 -05:00

(Migrated from github.com)

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

not really

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

karel-m commented

2018-03-25 15:27:05 -05:00

(Migrated from github.com)

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.

FYI: there are two related PR #366 and #367 I would like to propose changing: ```C int base16_decode(const char *in, unsigned char *out, unsigned long *outlen) ``` to: ```C 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`.