Re: Bug w/ gawk stat extension

[Glenn Zazulia]

On Jun 21, Andrew J. Schorr wrote:
> On Mon, Jun 20, 2005 at 03:10:22PM -0600, Glenn Zazulia wrote:
>> Great!  I thought I'd also send another slight fix in the same 
>> section of code:  pass in "sizeof buf - 1" to readlink() for the
>> bufsize arg to save room for appending the trailing NULL character.
>> This should prevent that fatal("size of symbolic link too big") failure
>> case from ever occurring.  This is just a boundary case in sample
>> extension code; so again, it's not a critical fix, but if you're
>> already updating the filefuncs.c file with the other fix, you might
>> as well include this too.  Attached is the complete patch, including
>> both fixes.
> 
> But does it make sense to truncate the link value silently in that case?
> Wouldn't it be better simply to omit the "linkval" field from the
> array instead of returning an incorrect value?
> 
> Regards,
> Andy

... and ...
On Tue, Jun 21, 2005 at 03:34:21PM +0200, Andreas Schwab replied:
> If you want to retrieve the full link text you have to read at least
> st_size bytes from the link.

... and then Andy replied with a sample patch implementing the st_size bytes
    improvement.

This is a nice improvement except that now there is a slight (though
unlikely) race condition where this new code would return an omitted
linkval if the symlink is replaced with a "larger" one between the time
of the lstat() call and the time of the readlink() call.  Actually, this
race condition has always been present, and even when the new symlink is
"smaller", it is arguably incorrect to be returning the new linkval with
the old stat data.  A decent, simple solution is not immediately obvious
to me, though.  One thought is to repeat the lstat() call after the
readlink() call and confirm that the second sbuf matches the first, and
if not, to restart do_stat().  The increased complexity may not be worth
it, though, for an unlikely race condition in sample extension code.

I actually just checked a couple of "ls" implementations (though I didn't
check for any possibly recently modified versions), and interestingly,
they not only don't attempt to handle this race condition, they also
actually implement the readlink() call very similarly to the previous
gawk implementation with my proposed change, except that they use PATH_MAX
instead of BUFSIZ*2 (though I think the value is effectively the same on
many systems).  In fact, there is even a comment in one version that
st_size can't always be relied upon because some automounters give
incorrect st_size for mount points (not that I'm clear about when a
symlink would also be a mount point).

So, I don't know which solution is better, though either one is better
than the previously released code.  Either way, thanks for incorporating
my initial fix and for addressing this second issue that I reported as
well.

Glenn