From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12505 Path: news.gmane.org!.POSTED!not-for-mail From: Dale Weiler Newsgroups: gmane.linux.lib.musl.general Subject: Re: stdio review Date: Tue, 13 Feb 2018 22:49:04 -0500 Message-ID: References: <20180213165953.GN1627@brightrain.aerifal.cx> <20180213192131.GO1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" X-Trace: blaine.gmane.org 1518580049 6335 195.159.176.226 (14 Feb 2018 03:47:29 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 14 Feb 2018 03:47:29 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-12522-gllmg-musl=m.gmane.org@lists.openwall.com Wed Feb 14 04:47:25 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1elo2M-0000oe-Mw for gllmg-musl@m.gmane.org; Wed, 14 Feb 2018 04:47:14 +0100 Original-Received: (qmail 20307 invoked by uid 550); 14 Feb 2018 03:49:17 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 20283 invoked from network); 14 Feb 2018 03:49:17 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=2JgYt4yK2+DadueG7wQ+dkWh63WhPN6z8v1Dk+X7+nE=; b=lyFO4aVgQvia+MhAVxZ9lA3NF7/X/DPPosdyEAdGSYI6guGX07HtkQKYYg6veh3v8f pvvOPeTOBRNgbxLEQVnJLvcQ/GWaEFri1a5T0IxEEfzQ3iXHRm53j7gTlwqm7fiswK/z j5NazCIHJ5nIqJLlomR7R7o6yARqeVstcLkk5ziT19rJWoqB5vw+guUiBd7C7yaKM3SY 7puQiXjH8TFWo/aLw4FAugVXwX3Wbhw+fekDSoMhQ/Tvu5GMthk86fP3kBLmuIy/zGoJ MqWEWY3kdaxHF8sibZTQ6TyjdbnDudWudVPXmB7hTXFWZr6POBdiR3HNSuZGHuwm0CTa sbbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=2JgYt4yK2+DadueG7wQ+dkWh63WhPN6z8v1Dk+X7+nE=; b=gf7XAtuZNew1ygUrCJyLbdYl6X2xtCM+5HmJxRGbSsovE9+sYkg1LSa1ymT5uEgcU9 jYL9UcdKfme3nsFsH8STMWRpGs9wb69LeDhNEDkH41dFPN6yPIPGz6c++yttF7dwzjcL OTET4kUrK9aN3B3D0RY07CeErIJDya0bjRz7h6zgGlFDug6keb+nDIlRQJpGMVuiPE0e eyEfFZA/ORLbLwqT35L2MXnXWw+AkLlAWh/VFeYIiSIsNv9yJA3F01U8TFyIUDRySqr5 hjXLXfHKDBdObfau7xI2o7cEHOOReUS+0l3WlzQxKkUCR7VcRtUGXKN8oO+4ZKAJPF58 28xQ== X-Gm-Message-State: APf1xPAwN0/vmWWDcQgpoJppEf0yh+R/073FG4kC52QPQr4PmklGffsd Ba4a3o6WggtozOWnAG8EgUymFJQAkKKqRUQt6jn3uaIP X-Google-Smtp-Source: AH8x2242nYZ193Cj3eiJb72jruBS9/m7WvQVC+4WCt/4UOdG83sZOT+pGlK37wHm0PW7qVDu9Z3fv11q3yHTQ6xFMBQ= X-Received: by 10.157.65.236 with SMTP id v41mr2351121oti.167.1518580144864; Tue, 13 Feb 2018 19:49:04 -0800 (PST) In-Reply-To: <20180213192131.GO1627@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:12505 Archived-At: >> fgetpos.c: >> fgetpos: [bug] >> using *(off_t*) to write _Int64 data into fpos_t structure when >> memcpy should be used, this breaks strict aliasing. Maybe add >> an off_t to the fpos_t union? > My leaning would be to add the off_t, but the type might not be > exposed and thus we would need to find a matching type that is > exposed. memcpy would be the nicest solution, but only if we had a way > of allowing the compiler to use builtin memcpy; otherwise it's a > gratuitous call. Seeing as the type _is_ exposed, adding the off_t to the union is likewise the nicest solution. Getting the compiler to use it's builtin memcpy, while using things like -fno-builtin seems more challenging here. If the type did need to be hidden, there's always the possibility of using the __may_alias__ stuff that memcpy/memset do but that seems more gratuitous to me. >> fmemopen.c: >> mseek: [style] >> It does goto upwards. > I guess you could call it that, but it's into a block with no path > out, so I don't think I would. It's one of the rarer instances where goto is used unconventionally. I say that because most uses of goto, especially in the case where they're in response to an error go down. > Compound literal table to reference whence as a lookup table > as a single expression. > I thought this was cute. It's definitely cute, but it does depend on the seek argument being one of the macro definitions in the [0, 2] range which I had to check, obviously those have no reason to ever be anything but those values; ABI and all but it's just additional mental load to ensure they weren't hence why I brought it up. >> fwrite.c: >> fwrite: [question] >> Should there be a check for size*nmemb overflow? > This is actually a complicated topic. Formally, I think the C standard > reads such that it would be valid for size*nmemb to exceed the size of > the data object to be written if you somehow know you'll hit a write > error before that happens. However real world implementations don't > work like that. In particular, the kernel will error out with EFAULT > if the buffer length extends past the valid userspace address range, > even if the writes would never happen; the only way to avoid this > would be to break longer-than-page writes down into separate > page-sized writes. So I think for practical purposes, we have to > interpret the standard as requiring that size*nmemb actually reflect > the size of the object passed in, and in that case, the multiplication > necessarily does not overflow. If there's an interpretation from WG14 > contrary to this, we'll have to revisit it. > See also https://sourceware.org/bugzilla/show_bug.cgi?id=19165 That is an interesting and somewhat odd edge case. Maybe for the time being a comment within here w.r.t it maybe needing to be revisited wouldn't hurt. In either case it doesn't appear to be harming anything. >> gets.c: >> gets: [optimize] >> The length of the string is calculated twice to strip the >> newline character off it. Why not rewrite it as: >> if (ret) { size_t i = strlen(s)-1; if (s[i] == '\n') s[i] = 0; } > Seriously, this is gets. It's always unsafe, deprecated, removed from > the current C standard. If it's gratuitously slow too, great. :-) Yes it's gets, but fixing it for O(n) instead of O(n*2) does make the musl static set slightly smaller, also makes programs using it crash twice as fast ;-) >> stdio_impl.h: [style] >> FUNLOCK macro has a trailing else which prompted me to look at every >> single instance of FUNLOCK to make sure all of them contained a >> semicolon. This is just dangerous, why not use the more common >> idiom of do { } while (0). > Indeed that should be fine. I think it's better understood by most folks as well, glad we're on the same page w.r.t this one. At least then you cannot fail to forget the semicolon. >> intscan.h: [style] >> It isn't apparent for why needs to be included. Should >> just forward declare struct FILE; here instead. > That would not work, because it's *not* struct FILE, it's FILE, which > happens to be defined as "struct _IO_FILE", but that's an > implementation detail. Including is the clean way to have > that. I don't understand why you couldn't replicate that behavior. It's what stdio.h already _does_ and seeing as the associated translation unit already includes stdio.h it seems gratuitously excessive. It's just an opaque pointer type being passed, how is a forward declaration incorrect. Does C distinguish between "opaque T" and "opaque T" with different underlying struct? If so I have many of code that needs to be changed on my end. >> floatscan.h: [style] >> It isn't apparent for why needs to be included. Should >> just forward declare struct FILE; here instead. > Same. Same