[Mauiusers] Snake biting its tail in PBS interface for building the host list...

Alexis Cousein al at sgi.com
Mon Mar 15 12:45:58 MDT 2010


A customer of mine was just trying Maui 3.3 recently, and Maui
didn't seem to communicate the host list to Torque properly
when it started a job (resulting in a broken $PBS_NODEFILE,
in this case containing only the entries for the last
host that would have been on the correct list).

In the series "Too smart for one's own good", I submit this snippet
of code:

       sprintf(TSBuf,"%s%s:ppn=%d",
         TSBuf,
         tmpHostName,
         NL[tindex].TC);

The idea is to concatenate to a string by first
printing itself onto itself (Ghee! Whiz!) and then
print something else at the end of the string once you've
encountered the '\0'.

Aside from being inefficient compared to simply skipping
to the end of the string and writing there,...

...unfortunately, not all glibc implementations actually appreciate
this. When you write %s in the format specifier, you're supposed to
pass a pointer to a constant array of characters. And obviously,
the string you're trying to write to is, by definition, NOT constant.

There are quite a lot of possible implementations that go beyond
the naive one-character-at-a-time-from-left-to-right that would
break with this.

In this case, I suspect sprintf in my glibc version actually hardened
itself against "writing itself" and simply did nothing for the
first %s specifier, which resulted in the string always
getting lost when the author tried to append. I haven't checked, but
all symptoms are consistent with this.

This patch:
--- MPBSI.c.orig        2010-02-19 22:14:21.000000000 +0200
+++ MPBSI.c     2010-03-15 16:23:11.000000000 +0200
@@ -6358,6 +6358,7 @@
    int tindex;

    char tmpHostName[MAX_MLINE];
+  char tmpTaskList[MAX_MLINE+10];

    mnode_t *N;

@@ -6410,10 +6411,10 @@
        }
      else
        {
-      sprintf(TSBuf,"%s%s:ppn=%d",
-        TSBuf,
+      snprintf(tmpTaskList,MAX_MLINE+10,"%s:ppn=%d",
          tmpHostName,
          NL[tindex].TC);
+       MUStrCat(TSBuf,tmpTaskList,BufSize);
        }
      }  /* END for (tindex) */



even though it's obviously devoid of the high-tech wizardry of the
original, does have the merit of actually working on my customer's
machine (with SLES10SP3).

Another variant would retain sprintf -- or rather, use snprintf
-- but pass TSBuf+strlen(TSBuf) -- or better, something using
strnlen -- as first argument to snprintf, which *also* gets rid
of having to write the string to itself.

But I haven't tried it: I find it a lot less readable, and
here I like the fact I use the obviously hardened MUStrCat used by the
rest of the routine to build TSBuf: what's sauce for the
goose is sauce for the gander.

By the way, I *really* dislike sprintf (without "n") with a vengeance
in code like this. It's a recipe for data corruption when some
things become longer than you'd expect.

--
Alexis Cousein
(in a private capacity)
al at sgi.com








More information about the mauiusers mailing list