Using the return value of strlcpy() and strlcat()

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:

[ let's convert
+		strlcpy(buf + buflen, name, NAMEDATALEN);
+		buflen += strlen(buf + buflen);
to
+		buflen += strlcpy(buf + buflen, name, NAMEDATALEN);
]

I don't think that's a safe transformation: what strlcpy returns is
strlen(src), which might be different from what it was actually
able to fit into the destination.

Sure, they're equivalent if no truncation occurred; but if we were
100.00% sure of no truncation, we'd likely not bother with strlcpy.

regards, tom lane

On Wed, Mar 13, 2019 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:

[ let's convert
+             strlcpy(buf + buflen, name, NAMEDATALEN);
+             buflen += strlen(buf + buflen);
to
+             buflen += strlcpy(buf + buflen, name, NAMEDATALEN);
]

I don't think that's a safe transformation: what strlcpy returns is
strlen(src), which might be different from what it was actually
able to fit into the destination.

Sure, they're equivalent if no truncation occurred; but if we were
100.00% sure of no truncation, we'd likely not bother with strlcpy.

So, if return value < length (3rd argument) we should be able to use the
return value and avoid the strlen, else do the strlen ?

Ashwin Agrawal <aagrawal@pivotal.io> writes:

On Wed, Mar 13, 2019 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't think that's a safe transformation: what strlcpy returns is
strlen(src), which might be different from what it was actually
able to fit into the destination.
Sure, they're equivalent if no truncation occurred; but if we were
100.00% sure of no truncation, we'd likely not bother with strlcpy.

So, if return value < length (3rd argument) we should be able to use the
return value and avoid the strlen, else do the strlen ?

Mmm ... if there's a way to do it that's not messy and typo-prone,
maybe. But I'm dubious that the potential gain is worth complicating
the code. The strings involved aren't usually all that long.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Ashwin Agrawal <aagrawal@pivotal.io> writes:

On Wed, Mar 13, 2019 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't think that's a safe transformation: what strlcpy returns is
strlen(src), which might be different from what it was actually
able to fit into the destination.

Yeah, Andrew Gierth pointed this out on IRC as well.

Sure, they're equivalent if no truncation occurred; but if we were
100.00% sure of no truncation, we'd likely not bother with strlcpy.

So, if return value < length (3rd argument) we should be able to use the
return value and avoid the strlen, else do the strlen ?

Mmm ... if there's a way to do it that's not messy and typo-prone,
maybe. But I'm dubious that the potential gain is worth complicating
the code. The strings involved aren't usually all that long.

Please consider this patch withdrawn.

- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen

On Fri, 15 Mar 2019 at 00:11, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:

Tom Lane <tgl@sss.pgh.pa.us> writes:

Mmm ... if there's a way to do it that's not messy and typo-prone,
maybe. But I'm dubious that the potential gain is worth complicating
the code. The strings involved aren't usually all that long.

Please consider this patch withdrawn.

Amusingly it seems the strlcpy() return value of the chars it would
have copied if it didn't run out of space is not all that useful for
us. I see exactly 2 matches of git grep "= strlcpy". The
isolation_init() one looks genuine, but the SerializeLibraryState()
looks a bit bogus. Looking at EstimateLibraryStateSpace() it seems it
estimates the exact space, so the strlcpy should never cut short, but
it does seem like a bad example to leave laying around.

We should have maybe thought a bit harder when we put that strlcpy
code into the codebase and considered if we might have been better off
inventing our own function that just returns what it did copy instead
of what it would have.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:

We should have maybe thought a bit harder when we put that strlcpy
code into the codebase and considered if we might have been better off
inventing our own function that just returns what it did copy instead
of what it would have.

Well, strlcpy is (somewhat) standardized; we didn't just invent it
off the cuff. I thought a little bit about whether it would be worth
having a variant version with a different return value, but concluded
that having YA strcpy variant would more likely be a dangerous source
of thinkos than something that was actually helpful. Otherwise I'd
have given Ashwin a more positive reaction...

regards, tom lane