[Bug]: Base16_Encode seems have a bug

[haquocviet]

trafficstars

Contact Details

No response

Version

5.4.0

Description

Base16_Encode always returns one additional byte more than the actual encoded bytes because the code near the end is:

 /* force 0 at this end */
    out[outIdx++] = 0;

This behavior has two issues:

• If the output buffer has room enough only to store the actual number of encoded bytes, then the additional byte shall be written outside the scope of the output buffer.
• Even if the output buffer has room enough to store more than the actual number of encoded bytes , the output size is still incorrect due to the additional byte.

I think this function should be fixed to behave the same as the Base64_Encode does.

Reproduction steps

No response

Relevant log output

No response

[dgarske]

@kareem-wolfssl please review the history for this null termination. I suspect it is there for a good reason, however we should have a run-time or build-time option to disable it.

[kareem-wolfssl]

Hi @haquocviet,

We do ensure the output buffer is large enough with this check:

if (*outLen < (2 * inLen + 1))
    return BAD_FUNC_ARG;

The +1 is for the terminating zero.

We are also correctly setting the size. We are considering this zero as part of the encoded buffer. The line that adds that zero is incrementing outIdx, then we set outLen to outIdx after:

/* force 0 at this end */
out[outIdx++] = 0;

*outLen = outIdx;

There is no reason that this null byte needs to be required, however. I can either add a flag to skip it unconditionally, or match DoBase64_Encode's behavior which only adds a null terminator if there's room in out. Which would you prefer for your use case?

[haquocviet]

Hello there, It's better to make Base16_Encode to behave the same way as DoBase64_Encode so that three things shall be guaranteed:

• The encoded size returned is correct (the current version isn't).
• The null should append to the output buffer if the buffer has enough room.
• The function will not return BAD_FUNC_ARG if the output buffer size is just enough for encoded data only.

So,

 if (*outLen < (2 * inLen + 1))
        return BAD_FUNC_ARG;

should be changed to:

 if (*outLen < (2 * inLen))
        return BAD_FUNC_ARG;

and

/* force 0 at this end */
    out[outIdx++] = 0;

becomes:

if (2 * inLen < outLen)
    out[outIdx] = 0;

And please do not consider the changes I suggest here just for my use case.

[haquocviet]

I'm noticed some changes you made but one of them still matters:

if (*outLen > outIdx)
        out[outIdx++]= '\0';

Could you explain why you count the null byte to the output length while the DoBase64_Encode does not. Is there any special reason to make two functions behave differently?

[kareem-wolfssl]

Good point, will update this behavior to match DoBase64_Encode's.