Re: [PATCH] Fix buffer not null terminated on (ecpg lib) - Mailing list pgsql-hackers

Hello.

At Wed, 22 Apr 2020 19:48:07 -0300, Ranier Vilela <ranier.vf@gmail.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.

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