Re: [tz] defensive value for define on Solaris causes load of timerule to fail
Retry, now to the mailing list as well.
So, FILENAME_MAX is probably the better choice, but may be, a
#if FILENAME_MAX < 260 #undef FILENAME_MAX #define FILENAME_MAX 260 #endif
code part is needed? Then FILENAME_MAX is at a defined minimum value (of course, the limit of 260 is also quite an arbitrary choice).
I don't like the idea of redefining `FILENAME_MAX`. Just use some reasonably large value of our choosing for the length of `fullname`. Even with a modern GNU/Linux `FILENAME_MAX` value of 4096 (on my system at least), the `fullname` member of `union local_storage` is smaller than the `u` member.
Anything else is also ok, but the union now does not have 'take largest value of somethong' for the fullname member. It is also ok for me to say: #if FILENAME_MAX < 260 #define TZ_FILENAME_MAX 260 #else #define TZ_FILENAME_MAX FILENAME_MAX #endif And use TZ_FILENAME_MAX in the union. My message is just to say 'please take care for platforms where FILENAME_MAX is arbitrary (too) short'. And the value of 14, as used on Solaris, is too short, as some time rules already exceed this length. It makes no sense (i.e. it does not help for the other platforms) in this case that Linux has a much bigger value for this define. As I have checked now, on most platforms, except Solaris, it is 260. Linux uses (as one of the few) a larger value. I've checked HPUX, AIX, Windows, Solaris and several Linux flavors. Kees
#if FILENAME_MAX < 260 #define TZ_FILENAME_MAX 260 #else #define TZ_FILENAME_MAX FILENAME_MAX #endif
Better approach: #if !defined(FILENAME_MAX) || FILENAME_MAX < 260 // e.g. Solaris uses a too small value to hold a path #define TZ_FILENAME_MAX 260 #else #define TZ_FILENAME_MAX FILENAME_MAX #endif /* Local storage needed for 'tzloadbody'. */ union local_storage { /* The file name to be opened. */ char fullname[TZ_FILENAME_MAX + 1]; /* The results of analyzing the file's contents after it is opened. */ struct { /* The input buffer. */ union input_buffer u; /* A temporary state used for parsing a TZ string in the file. */ struct state st; } u; };
On 12/06/17 14:37, Kees Dekker wrote:
#if FILENAME_MAX < 260 #define TZ_FILENAME_MAX 260 #else #define TZ_FILENAME_MAX FILENAME_MAX #endif
Better approach:
#if !defined(FILENAME_MAX) || FILENAME_MAX < 260 // e.g. Solaris uses a too small value to hold a path #define TZ_FILENAME_MAX 260 #else #define TZ_FILENAME_MAX FILENAME_MAX #endif
/* Local storage needed for 'tzloadbody'. */ union local_storage { /* The file name to be opened. */ char fullname[TZ_FILENAME_MAX + 1];
/* The results of analyzing the file's contents after it is opened. */ struct { /* The input buffer. */ union input_buffer u;
/* A temporary state used for parsing a TZ string in the file. */ struct state st; } u; };
It looks reasonable to me, although checking whether FILENAME_MAX is defined is not technically necessary. I'm sure Paul will code whatever seems appropriate! -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Web: http://www.mev.co.uk/ )=-
On 06/12/2017 07:00 AM, Ian Abbott wrote:
I'm sure Paul will code whatever seems appropriate!
Thanks, I gave it a shot by installing the attached, which avoids FILENAME_MAX entirely. In practice the new length should be good enough.
On Jun 12, 11:21am, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] defensive value for define on Solaris causes load of tim | return EINVAL; | if (sizeof lsp->fullname - 1 <=3D strlen(p) + strlen(name)) | return ENAMETOOLONG; | - strcpy(fullname, p); | - strcat(fullname, "/"); | - strcat(fullname, name); | + strcpy(lsp->fullname, p); | + strcat(lsp->fullname, "/"); | + strcat(lsp->fullname, name); Why not: snprintf(lsp->fullname, sizeof(lsp_fullname), "%s/%s", p, name); and remove more code... christos
On 2017-06-12 15:10, Christos Zoulas wrote:
On Jun 12, 11:21am, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] defensive value for define on Solaris causes load of tim
| return EINVAL; | if (sizeof lsp->fullname - 1 <=3D strlen(p) + strlen(name)) | return ENAMETOOLONG; | - strcpy(fullname, p); | - strcat(fullname, "/"); | - strcat(fullname, name); | + strcpy(lsp->fullname, p); | + strcat(lsp->fullname, "/"); | + strcat(lsp->fullname, name);
Why not:
snprintf(lsp->fullname, sizeof(lsp_fullname), "%s/%s", p, name);
and remove more code... From another post today: /* ** We avoid using snprintf since it's not available on all systems. */
-- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
Well, if the strcpy/strcat/strcat sequence is safe, the corresponding sprintf is also safe (and if it wasn't safe, then the code would need to be fixed anyway). So it's permissible and safe to use: sprintf(lsp->fullname, "%s/%s", p, name); I was under the impression Paul had just said that using snprintf() was OK now as it was part of C99 — though he mistyped C99 as C89 in the last email I saw commenting on the subject. On Mon, Jun 12, 2017 at 3:41 PM, Brian Inglis < Brian.Inglis@systematicsw.ab.ca> wrote:
On 2017-06-12 15:10, Christos Zoulas wrote:
On Jun 12, 11:21am, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] defensive value for define on Solaris causes load of tim
| return EINVAL; | if (sizeof lsp->fullname - 1 <=3D strlen(p) + strlen(name)) | return ENAMETOOLONG; | - strcpy(fullname, p); | - strcat(fullname, "/"); | - strcat(fullname, name); | + strcpy(lsp->fullname, p); | + strcat(lsp->fullname, "/"); | + strcat(lsp->fullname, name);
Why not:
snprintf(lsp->fullname, sizeof(lsp_fullname), "%s/%s", p, name);
and remove more code... From another post today: /* ** We avoid using snprintf since it's not available on all systems. */
-- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
-- Jonathan Leffler <jonathan.leffler@gmail.com> #include <disclaimer.h> Guardian of DBD::Informix - v2015.1101 - http://dbi.perl.org "Blessed are we who can laugh at ourselves, for we shall never cease to be amused."
Date: Mon, 12 Jun 2017 16:25:56 -0700 From: Jonathan Leffler <jonathan.leffler@gmail.com> Message-ID: <CAH+RLGERDa10n6aNpyqoQ60jxWhsK_sWrjLcgykyjhVpwD=HRg@mail.gmail.com> | So it's permissible and safe to use: | | sprintf(lsp->fullname, "%s/%s", p, name); Actually in that case, the snprintf is better than either the scrcpy/strcat version or the sprintf() version, as after this recent change, sizeof(isp->fullname) (ie: the buffer space available) is based upon the sizes of other unrelated objects, and perhaps somewhere, on some system might not be big enough. kre ps: All I did right now was read the diffs, so if the size will be guaranteed to be big enough because of what it is based upon, then fine.
On 06/12/2017 04:25 PM, Jonathan Leffler wrote:
I was under the impression Paul had just said that using snprintf() was OK now as it was part of C99 — though he mistyped C99 as C89
Oops, I thought that snprintf was in C89. I guess if we're still assuming only C89 or better then that's a bug that should be fixed. Please see attached proposed patch (parts 3 and 4, the latter to fix a typo in 3). For localtime.c, it's probably better to avoid sprintf/snprintf/etc. In the good old days people would avoid sprintf because it pulled in a bunch of floating-point code (to support %f etc.) even when not needed. Nowadays the major knock on sprintf/etc. is that it doesn't work if the result contains more than INT_MAX bytes. That being said, strcat should be avoided there, if only to pacify picky compilers and human readers that worry about a buffer overrun that cannot really happen. I installed the attached patches 1 and 2 to try to make progress in that area.
Why not:
snprintf(lsp->fullname, sizeof(lsp_fullname), "%s/%s", p, name);
and remove more code...
Just my 2 cents: looks well, but there are platforms (like MS Window/Visual Studio, see MSDN on snprintf :-() that do not guarantee termination of the string if a buffer is too small. To be bulletproof, add lsp->fullname[sizeof(lsp->fullname)-1]='\0'; Kees
On 12/06/17 14:34, Kees Dekker wrote:
Retry, now to the mailing list as well.
So, FILENAME_MAX is probably the better choice, but may be, a
#if FILENAME_MAX < 260 #undef FILENAME_MAX #define FILENAME_MAX 260 #endif
code part is needed? Then FILENAME_MAX is at a defined minimum value (of course, the limit of 260 is also quite an arbitrary choice).
I don't like the idea of redefining `FILENAME_MAX`. Just use some reasonably large value of our choosing for the length of `fullname`. Even with a modern GNU/Linux `FILENAME_MAX` value of 4096 (on my system at least), the `fullname` member of `union local_storage` is smaller than the `u` member.
Anything else is also ok, but the union now does not have 'take largest value of somethong' for the fullname member. It is also ok for me to say:
#if FILENAME_MAX < 260 #define TZ_FILENAME_MAX 260 #else #define TZ_FILENAME_MAX FILENAME_MAX #endif
And use TZ_FILENAME_MAX in the union.
My message is just to say 'please take care for platforms where FILENAME_MAX is arbitrary (too) short'. And the value of 14, as used on Solaris, is too short, as some time rules already exceed this length. It makes no sense (i.e. it does not help for the other platforms) in this case that Linux has a much bigger value for this define. As I have checked now, on most platforms, except Solaris, it is 260. Linux uses (as one of the few) a larger value. I've checked HPUX, AIX, Windows, Solaris and several Linux flavors.
I mainly mentioned the 4096 value to say that even that high a value has no impact on the overall size of `union local_storage`, so there is scope for being quite generous (rather than picking a lower value such as 260). If the OS can't handle opening filenames of that length, it doesn't really matter as it just changes whereabouts in the code the ENAMETOOLONG error number is produced. -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Web: http://www.mev.co.uk/ )=-
participants (7)
-
Brian Inglis -
christos＠zoulas.com -
Ian Abbott -
Jonathan Leffler -
Kees Dekker -
Paul Eggert -
Robert Elz