[semi-OT] a hunk of not-very-portable code I've been working on

[Seebs]

The code here is mostly off-topic, but I bring it up because a few people
have expressed interest, and while a great deal of the code is non-portable,
there's also a fairly large amount of basically generic C involved.

http://github.com/wrpseudo/pseudo

This is a pretty heavily Linux-specific thing (it could probably be ported to
other Unixes with reasonable effort, but isn't remotely possible for non-Unix,
I'd guess) that I've done at various times over the last year and a half.
I'd guess that in total it represents a good two months' worth of effort,
possibly more if you include some of the more arcane debugging. With the
exception of one routine that was added while I was on vacation, essentially
the entire thing is my code, though.

And yes, some of it's pretty ugly. One of the reasons I was pushing for
an open source release is that I wanted to have some incentive for us to
fix things, and indeed, it just went through some noticeable cleanup.

Of possible interest to comp.lang.c users is a fair amount of string
manipulation code, some string manipulation, and one of the very rare
cases you'll see where
#include "foo.c"
is not a mistake. (It may be a poor design decision, but it's worked pretty
well so far.)

Comments and feedback welcome, whether here or elsewhere. Obviously,
comments specific to the dynamic linker magic, filesystem code, and such
are probably better sent via email or posted as comments on the github
page, but I'd be happy to answer questions about the C aspects of this,
whether it's "how did you think of this" or "what the hell were you thinking".
Bug reports are also quite welcome.

So far as I know, this code is essentially clean C99, except for the intrinsic
OS dependencies. I haven't *intentionally* used any GNU extensions or
anything else, but I haven't been able to check that as carefully as I might
like, because requesting a pure C compile disables all the OS-specific stuff
that is the actual functionality of the program.

-s

 

[Ersek, Laszlo]

http://github.com/wrpseudo/pseudo

LGPL, wow, very generous!

Comments and feedback welcome, whether here or elsewhere. Obviously,
comments specific to the dynamic linker magic, filesystem code, and such
are probably better sent via email or posted as comments on the github
page, but I'd be happy to answer questions about the C aspects of this,
whether it's "how did you think of this" or "what the hell were you thinking".
Bug reports are also quite welcome.

I hate to be that guy, but... pseudo_server.c, lines 131-133:

pseudo_path = pseudo_prefix_path(PSEUDO_PIDFILE);
fp = fopen(pseudo_path, "w");
fprintf(fp, "%d\n", getpid());

a) I think fopen() may return 0.

b) Furthermore, getpid() returns pid_t, not int, so you should convert
it to intmax_t and print it accordingly. (You said C99.)

c) Looking at pseudo_prefix_path() in pseudo_util.c, it may
theoretically return 0 if malloc() (or strdup()) returns 0.

d) Worse, if the user exports PSEUDO_PREFIX='' for the process, then
prefix_len will be set to (size_t)0, and then comes

while ((prefix[prefix_len - 1] == '/') && (prefix_len > 0)) {

which accesses prefix[-1] or prefix[(size_t)-1], dependent on whether
size_t is promoted to int *or* int is converted to size_t.


I apologize in case I didn't notice if these circumstances are
impossible for some reasons. I was too lazy to build the package and
check if these bugs really exist; sorry.

lacos

 

[Seebs]

LGPL, wow, very generous!

It was GPLv2 before, but we decided to bump it over before the release. I
think there was some project that had expressed slight interest, but wanted
LGPL.

I hate to be that guy, but...

No, that's awesome. It's why we did this. Well, that, and we just wanted
to be friendly.

pseudo_server.c, lines 131-133:

pseudo_path = pseudo_prefix_path(PSEUDO_PIDFILE);
fp = fopen(pseudo_path, "w");
fprintf(fp, "%d\n", getpid());

a) I think fopen() may return 0.
D'oh!

b) Furthermore, getpid() returns pid_t, not int, so you should convert
it to intmax_t and print it accordingly. (You said C99.)

Good catch. You can tell, probably, that I've been writing code using
getpid() since before pid_t existed. (That said, I'd probably just
convert to "long long", since I think we do have some systems too old
for intmax_t.)

c) Looking at pseudo_prefix_path() in pseudo_util.c, it may
theoretically return 0 if malloc() (or strdup()) returns 0.
Ayup.

d) Worse, if the user exports PSEUDO_PREFIX='' for the process, then
prefix_len will be set to (size_t)0, and then comes

while ((prefix[prefix_len - 1] == '/') && (prefix_len > 0)) {

which accesses prefix[-1] or prefix[(size_t)-1], dependent on whether
size_t is promoted to int *or* int is converted to size_t.

Good catch. It hadn't occurred to me, just because, in the intended
usage, you could never make that work. I should probably sanity check
that PSEUDO_PREFIX must be an absolute path.

And yes, I think you're right on all of these. What's scary is that we've
never seen a failure of this code, and that hunk of code has been reviewed
at least twice. (And not by bad reviewers, either!)

For reference, an earlier, and buggier, version of pseudo has been in
production use for over a year now, and we've had exactly one actual
bug in the field, and it wasn't this. Go figure.

Thanks. I filed a bug report on this in the internal tracker.

-s

 

[Ersek, Laszlo]

d) Worse, if the user exports PSEUDO_PREFIX='' for the process, then
prefix_len will be set to (size_t)0, and then comes

while ((prefix[prefix_len - 1] == '/') && (prefix_len > 0)) {

which accesses prefix[-1] or prefix[(size_t)-1], dependent on whether
size_t is promoted to int *or* int is converted to size_t.

Sorry, I didn't even read past the closing bracket, now it seems obvious
that you inadvertently reversed the two sides of the && operator.

e) (style note) Still in pseudo_prefix_path(), you may want to move the
definition of "path" into the narrowest possible block, that is, where
it is assigned to. (And then the separate definition and assignment can
be merged into an initialization, since your style seems to allow that.)

lacos

 

[Seebs]

d) Worse, if the user exports PSEUDO_PREFIX='' for the process, then
prefix_len will be set to (size_t)0, and then comes
while ((prefix[prefix_len - 1] == '/') && (prefix_len > 0)) {
which accesses prefix[-1] or prefix[(size_t)-1], dependent on whether
size_t is promoted to int *or* int is converted to size_t.

Sorry, I didn't even read past the closing bracket, now it seems obvious
that you inadvertently reversed the two sides of the && operator.

Actually, no. I just didn't think about the zero-length case at all. But
as you note, reversing this would help. There's another problem -- I don't
think I should be modifying the string returned by getenv().

Yeek. This is more wrong than it looked.

This is why releasing code is so very useful.

(BTW, the errors you're seeing here are fairly typical. For whatever
reason, I can get moderately complicated stuff right pretty often, but
really obvious stuff tends to elude me.)

e) (style note) Still in pseudo_prefix_path(), you may want to move the
definition of "path" into the narrowest possible block, that is, where
it is assigned to. (And then the separate definition and assignment can
be merged into an initialization, since your style seems to allow that.)

I think in some earlier version, path was used in more possible cases.

-s

 

[Moi]

LGPL, wow, very generous!



I hate to be that guy, but... pseudo_server.c, lines 131-133:

pseudo_path = pseudo_prefix_path(PSEUDO_PIDFILE); fp =
fopen(pseudo_path, "w");
fprintf(fp, "%d\n", getpid());

More or less the same thing with getsizes:

printf("ThingyMember: %d\n" , offsetof(struct message, member));
printf("Whole Thing: %d\n" , sizeof(struct message));

-->> either %z or %u and cast to unsigned, or just cast to int.


AvK

 

[Flash Gordon]

The code here is mostly off-topic, but I bring it up because a few people
have expressed interest, and while a great deal of the code is non-portable,
there's also a fairly large amount of basically generic C involved.

http://github.com/wrpseudo/pseudo

<snip>

Your date/time formats in psuedolog.c (and hence probably the rest of
the code) are somewhat US-centric... "%x %X" could be useful.

I don't have the energy this evening to review things properly.

 

[Seebs]

Your date/time formats in psuedolog.c (and hence probably the rest of
the code) are somewhat US-centric... "%x %X" could be useful.

There's some of that, although I thought I'd made sure %x %X was at
least one of the accepted formats.

I don't have the energy this evening to review things properly.

No worries, I mostly just posted it 'cuz I had a blast working on it,
and I thought it was pretty cool that my employer was willing to give
it away free.

Today's project has been adding support for emulating chroot(). It's
working in very light testing, but not all the way there yet. (Once
that's done, we'll stop using fakechroot. I then have one other thing,
a local hunk called "fakepasswd", to emulate, and we'll have reduced
the complexity of a large chunk of our build system dramatically.)

There's a fair amount of Very Non Portable Magic there, but the
fundamental guts of that hunk of work is a moderately generalizable
question. Imagine that you want to wrap some existing code such that,
say, a given value is automatically appended to strings it produces,
or removed from strings handed to it. How do you do this? Who owns
any newly allocated storage? etc.

-s

 

[spinoza1111]

d) Worse, if the user exports PSEUDO_PREFIX='' for the process, then
prefix_len will be set to (size_t)0, and then comes

   while ((prefix[prefix_len - 1] == '/') && (prefix_len > 0)) {

which accesses prefix[-1] or prefix[(size_t)-1], dependent on whether
size_t is promoted to int *or* int is converted to size_t.

Sorry, I didn't even read past the closing bracket, now it seems obvious
that you inadvertently reversed the two sides of the && operator.

Please see http://groups.google.com.hk/group/comp.lang.c/msg/dc8d911ed3848018?hl=en.

 

[spinoza1111]

d) Worse, if the user exports PSEUDO_PREFIX='' for the process, then
prefix_len will be set to (size_t)0, and then comes
   while ((prefix[prefix_len - 1] == '/') && (prefix_len > 0)) {
which accesses prefix[-1] or prefix[(size_t)-1], dependent on whether
size_t is promoted to int *or* int is converted to size_t.

Sorry, I didn't even read past the closing bracket, now it seems obvious
that you inadvertently reversed the two sides of the && operator.

Please seehttp://groups.google.com.hk/group/comp.lang.c/msg/dc8d911ed3848018?hl=en.

e) (style note) Still in pseudo_prefix_path(), you may want to move the
definition of "path" into the narrowest possible block, that is, where
it is assigned to. (And then the separate definition and assignment can
be merged into an initialization, since your style seems to allow that.)

lacos

Also see http://groups.google.com.hk/group/comp.lang.c/msg/3d31fea1616c1c4d?hl=en

 

[Moi]

The code here is mostly off-topic, but I bring it up because a few
people have expressed interest, and while a great deal of the code is
non-portable, there's also a fairly large amount of basically generic C
involved.

http://github.com/wrpseudo/pseudo

This is a pretty heavily Linux-specific thing (it could probably be
ported to other Unixes with reasonable effort, but isn't remotely
possible for non-Unix, I'd guess) that I've done at various times over
the last year and a half. I'd guess that in total it represents a good
two months' worth of effort, possibly more if you include some of the
more arcane debugging. With the exception of one routine that was added
while I was on vacation, essentially the entire thing is my code,
though.

And yes, some of it's pretty ugly. One of the reasons I was pushing for
an open source release is that I wanted to have some incentive for us to
fix things, and indeed, it just went through some noticeable cleanup.

Of possible interest to comp.lang.c users is a fair amount of string
manipulation code, some string manipulation, and one of the very rare
cases you'll see where
#include "foo.c"
is not a mistake. (It may be a poor design decision, but it's worked
pretty well so far.)

Comments and feedback welcome, whether here or elsewhere. Obviously,
comments specific to the dynamic linker magic, filesystem code, and such
are probably better sent via email or posted as comments on the github
page, but I'd be happy to answer questions about the C aspects of this,
whether it's "how did you think of this" or "what the hell were you
thinking". Bug reports are also quite welcome.

So far as I know, this code is essentially clean C99, except for the
intrinsic OS dependencies. I haven't *intentionally* used any GNU
extensions or anything else, but I haven't been able to check that as
carefully as I might like, because requesting a pure C compile disables
all the OS-specific stuff that is the actual functionality of the
program.

In pseudolog.c (line 619) you assign the result of snprintf() to
a variable of type size_t. snprintf() returns int, and may return -1
(but that is very unlikely, I agree)

I usually use the ugly construct:

int rc;
rc = snprintf( buff, sizeof buff, ...);
if (rc < 0 || rc >= sizeof buff -1) { ... error }
else { real_len= rc; ... }

HTH,
AvK