Alexis,
Just letting you know that I checked your fix into trunk. Let me know if
you need a snapshot.
Thanks,
Brian
On 03/15/2010 12:45 PM, Alexis Cousein wrote:
> 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)
> [email protected]
>
>
>
>
>
>
> _______________________________________________
> mauiusers mailing list
> [email protected]
> http://www.supercluster.org/mailman/listinfo/mauiusers
>
_______________________________________________
mauiusers mailing list
[email protected]
http://www.supercluster.org/mailman/listinfo/mauiusers
