Re: [PATCH] Fix buffer not null terminated on (ecpg lib)

Em qua., 22 de abr. de 2020 às 23:27, Kyotaro Horiguchi <
horikyota(dot)ntt(at)gmail(dot)com> escreveu:

> Hello.
>
> At Wed, 22 Apr 2020 19:48:07 -0300, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
> wrote in
> > Hi,
> > strncpy, it is not a safe function and has the risk of corrupting memory.
> > On ecpg lib, two sources, make use of strncpy risk, this patch tries to
> fix.
> >
> > 1. Make room for the last null-characte;
> > 2. Copies Maximum number of characters - 1.
> >
> > per Coverity.
>
> - strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate));
> + sqlca->sqlstate[sizeof(sqlca->sqlstate) - 1] = '\0';
> + strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate) - 1);
>
> Did you look at the definition and usages of the struct member?
> sqlstate is a char[5], which is to be filled with 5-letter SQLSTATE
> code not terminated by NUL, which can be shorter if NUL is found
> anywhere (I'm not sure there's actually a case of a shorter state
> code). If you put NUL to the 5th element of the array, you break the
> content. The existing code looks perfect to me.
>
Sorry, you are right.

>
> - strncpy(sqlca->sqlerrm.sqlerrmc, message,
> sizeof(sqlca->sqlerrm.sqlerrmc));
> - sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] = 0;
> + sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] =
> '\0';
> + strncpy(sqlca->sqlerrm.sqlerrmc, message,
> sizeof(sqlca->sqlerrm.sqlerrmc) - 1);
>
> The existing strncpy then terminating by NUL works fine. I don't think
> there's any point in doing the reverse way. Actually
> sizeof(sqlca->sqlerrm.sqlerrmc) - 1 is enough for the length but the
> existing code is not necessarily a bug.
>
Without understanding then, why Coveriy claims bug here.

regards,
Ranier Vilela