From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf35.google.com (mail-qv1-xf35.google.com [IPv6:2607:f8b0:4864:20::f35]) by sourceware.org (Postfix) with ESMTPS id B89C2385840D for ; Sat, 20 Nov 2021 14:42:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B89C2385840D Received: by mail-qv1-xf35.google.com with SMTP id b17so9146858qvl.9 for ; Sat, 20 Nov 2021 06:42:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=KTg3ubKz1Ih4dMxTnA7dJJtf0MsDE+9XjCi058C5rhw=; b=Y2lHIxLl4vdGabt3Yitn+n1M42R/QL2cXmtq/mMP61svS3epR7PU+GtYrT3gduOa+n zPh5nfHDN9mQNtrcfdYGFonBQctSbjwRmoTDOcDIlw24jt5AyaXz0zeuPDThu0+XCtq1 ueufzdsIOpbgv64KI18yTePvui6oZtkmbt449xK4Yj/FLcZzDbLU++hptNz5OrzlhNVa 3EqElDwpp5GS5HdN9HbNAAVQnOD0+cXEhbET2ZPfOrvdnGrO9veWa7F6AKyKfu/GoQKX +/kuR/Ov6L2F7biH3GuOHuzHpNNeEhkvIYJODU73QjJ1G7Db3rAIqVP/iN7hYtZGhcdF Ogmg== X-Gm-Message-State: AOAM530ZlwQupg6iESqQPxeqr+ffVTwPpEJcmdNu0PXJya0/rAaF42Do bH63GMXxmMFhCrdlgyhuziNl/LutBWHZFg89I80= X-Google-Smtp-Source: ABdhPJw3uwwnMiLtsKwA1e6t0lI0iKRqIiv4z5fnsFxlH1d6yhjOqQPbx7VwW/xOgEdOZp4erue+aOQ6aCThvDcMsrw= X-Received: by 2002:ad4:5965:: with SMTP id eq5mr83700845qvb.64.1637419328278; Sat, 20 Nov 2021 06:42:08 -0800 (PST) MIME-Version: 1.0 References: <20211119181524.3190417-1-alex@linutronix.de> In-Reply-To: From: Khem Raj Date: Sat, 20 Nov 2021 06:41:41 -0800 Message-ID: Subject: Re: [PATCH] debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t To: Alexander Kanavin Cc: =?UTF-8?Q?=C3=89rico_Nogueira?= , Mark Wielaard , elfutils-devel@sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Nov 2021 14:42:10 -0000 On Sat, Nov 20, 2021 at 12:11 AM Alexander Kanavin wrote: > > On Sat, 20 Nov 2021 at 05:13, =C3=89rico Nogueira w= rote: >> >> For what it's worth, most of the time64 support patches that I have seen >> use "%lld" and `long long` as the type for portable representation of >> time, instead of intmax_t, but each should work just as well as the >> other. > > > My original version did use %lld, but Khem tweaked it to use intmax_t. Pe= rhaps he can comment? intmax_t is not particularly bounded like long long int and sounded the right choice here say if we have 128bit machines in future. > >> >> Might be worth mentioning in the commit that musl 1.2.0 and above uses >> time64 on all platforms, and that glibc 2.34 has added support for >> time64 (which might be enabled by distro-wide CFLAGS), with possibly >> 2.35 or 2.36 making it enabled by default. > > > Yes, I can add that. > >> >> This is the wrong fix, a temporary variable should be used. When time_t >> is still 32 bits, it means you can accidentally write out of bounds for >> times after 2038 on little endian plaforms; and on big endian platforms, >> it's always writing out of bounds (and *only out of bounds*, before >> 2038, so you can't even access the result of fscanf). yes it seems that way, thanks for catching it.` >> >> Having now taken a second look at the code, I don't think this needs to >> be treated as time_t, anyway? cache_config is a `long`, and it's reading >> a max timeout value, which is unlikely to go beyond the hundreds of >> seconds... Given that the function returns an `int`, I'd even consider >> making `cache_config` an `int` directly. > > > I can fix this as well if Khem confirms. treating int is fine. debuginfod_config_cache() is still passed time_t as s= econd param in few call sites which are assigned to cache_config here, so we will still need to convert/truncate that assignment. > > Thanks for the review. > > Alex