[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20110713135423.GA23183@openwall.com>
Date: Wed, 13 Jul 2011 17:54:23 +0400
From: Solar Designer <solar@...nwall.com>
To: musl@...ts.openwall.com
Subject: Re: cluts review
On Wed, Jul 13, 2011 at 02:57:21PM +0200, Luka Mar??eti?? wrote:
> I guess I like doing things manually.
Sure. I also don't blindly apply patches that people send me for my
code. But I don't think there's a better way to represent the changes
being proposed.
> But thanks for the patch. Anyway, here:
> https://github.com/lmarcetic/cluts/commit/7c836ff779c1f9ffecdae9f7d469772e88d3bc68
OK. Why the "#define _POSIX_C_SOURCE 200809L //sigaction" vs. "#define
_XOPEN_SOURCE //sigaction" inconsistency, though? I think _XOPEN_SOURCE
is a safer bet here. And I generally avoid C++ comments in C source
files, even though this became legal in C99 (and many C compilers
recognized C++ comments before then). If we enable/require C99 anyway,
this is a non-issue, and perhaps I am too old-fashioned. ;-) Testing
cluts on some older systems could make sense, though - not so much to
test those systems' libc's, but rather to better test cluts itself.
> (note also that cluts.c should now return correct nr. of failed tests,
> that would've been a valid critique)
Yes, I noticed weird output there. Thanks for fixing it.
> >What you could actually want to do is get rid of the dependency on
> >PATH_MAX and FILENAME_MAX. The system does not guarantee that actual
> >pathnames fit in PATH_MAX anyway. So you may want to replace those
> >strcat()'s with dynamic memory allocation or add a check for potential
> >buffer overflow there (then report the error and skip the test).
>
> Ah, wherever I do this, I think the path isn't big anyway (cluts.c and
> buf.c). I'll keep it in mind if there's ever a chance they might be. Agreed?
I don't strictly object to this, but I am trying to help you improve
your code quality overall. It may be preferable to consistently apply
best practices to cluts source code rather than to take shortcuts in
places where this is currently deemed acceptable.
That said, I agree that you have other priorities right now than dealing
with those existing pathname length issues.
> I already have my sreturnf function for such purposes (right now it uses
> vsnprintf).
Oh, I didn't notice this one yet. Thanks for pointing it out.
> Why do you think *snprintf and *asprintf aren't portable?
They just are less portable than older functions such as sprintf().
In practice, I think *snprintf() are portable enough these days and for
this specific application, so your use of vsnprintf() is fine (although
calling it with "NULL, 0" is a stress-test). asprintf() may or may not
be portable enough depending on what systems cluts is meant to run on.
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
