Обсуждение: Avoid an odd undefined behavior with memcmp (src/bin/pg_rewind/pg_rewind.c)

Hi Ranier,



> IMO, I think that pg_rewind can have a security issue,
> if two files are exactly the same, they are considered different.
> Because use of structs with padding values is unspecified.
Logically you are right. But I don't understand what scenario
would require memcmp to compare ControlFileData.
In general, we read ControlFileData from a pg_control file
and then use members of ControlFileData directly.
So the two ControlFileData are not directly compared by byte.

> Fix by explicitly initializing with memset to avoid this.
And, even if there are scenarios that use memcmp comparisons,
your modifications are not complete.
There are three calls to the digestControlFile in the main()
of pg_rewind.c, and as your said(if right), these should do
memory initialization every time.





--
Best Regards,

Long

On Wed, 29 May 2024 at 07:02, Ranier Vilela <ranier.vf@gmail.com> wrote:
> The function *perform_rewind* has an odd undefined behavior.
> The function memcmp/, compares bytes to bytes.
>
> IMO, I think that pg_rewind can have a security issue,
> if two files are exactly the same, they are considered different.
> Because use of structs with padding values is unspecified.
>
> Fix by explicitly initializing with memset to avoid this.

It's unclear to me why you think this makes it any safer.  If you look
at the guts of digestControlFile(), you'll see that the memory you've
just carefully memset to zero is subsequently overwritten by a
memcpy().  Do you not notice that? or do you think memcpy() somehow
has some ability to skip over padding bytes and leave them set to the
value they were set to previously? It doesn't.

If your patch fixes the Coverity warning, then that's just a
demonstration of how smart the tool is.  A warning does not mean
there's a problem.

The location where we should ensure the memory is zeroed is when
writing the control file.  That's done in InitControlFile(). If you
look in there you'll see "memset(ControlFile, 0,
sizeof(ControlFileData));"

It would be great if you add a bit more thinking between noticing a
Coverity warning and raising it on the mailing list. I'm not sure how
much time you dwell on these warnings before raising them here, but
certainly, if we wanted a direct tie between a Coverity warning and
this mailing list, we'd script it. But we don't, so we won't. So many
of your reports appear to rely on someone on the list doing that
thinking for you. That does not scale well when in the majority of the
reports there's no actual problem. Please remember that false reports
wastes people's time.

If you're keen to do some other small hobby projects around here, then
I bet there'd be a lot of suggestions for things that could be
improved. You could raise a thread to ask for suggestions for places
to start.  I don't think your goal should be that Coverity runs on the
PostgreSQL source warning free. If that was a worthy goal, it would
have happened a long time ago.

David