diff mbox series

nss_readline: move the NUL terminator along with leading-whitespace strip

Message ID CAPinm07sZDi2R3YX7Js6x3o=fLPu5roGkSWtUpo1ua=oRqCgww@mail.gmail.com (mailing list archive)
State Failed CI
Delegated to: Avinal Kumar
Headers
Series nss_readline: move the NUL terminator along with leading-whitespace strip |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent
redhat-pt-bot/TryBot-32bit fail Patch series failed to apply

Commit Message

Ekalabya Ghosh Sept. 26, 2026, 3:18 a.m. UTC 
  From: Ekalabya Ghosh <ekalabya2010@gmail.com>
Date: Sat, 26 Sep 2026 08:47:45 +0530
Subject: [PATCH] nss_readline: move the NUL terminator along with
leading-whitespace strip

__nss_readline() strips leading whitespace from a freshly-read
database line by moving the remainder forward with
"memmove (buf, p, strlen (p))". This moves the line's content but
not its NUL terminator, which stays behind at its original, farther
right position (the line's original, pre-strip length). The bytes
between the newly shortened content and that stale terminator are the
tail end of the original (pre-shift) line -- since the memmove only
overwrites strlen (p) bytes starting at buf, positions at or beyond
that offset keep whatever was already there, and the real terminator
is further right than that. Reading the returned buffer back (e.g.
via strlen) therefore includes those leftover bytes, appended after
the line's own content.

Concretely: for the two-space-indented line " hello\n" in an
8-plus-byte buffer, after stripping the leading whitespace the
intended result is "hello\n" (6 bytes), but the buffer's terminator
stays at its original offset (8), so bytes 6-7 -- which happen to
still hold "o\n", the tail of the pre-strip line -- are included,
and the caller sees "hello\no\n" (8 bytes) instead.

This affects the last line of any nss_files-backed database
(passwd, group, hosts, etc.) that has leading whitespace, appending
extra, leftover bytes from the original line to the value the caller
receives. It stays within the buffer (bounded by the earlier
"buf[len - 1] == '\xff'" truncation check), so this is a
data-correctness bug, not a memory-safety one.

Move strlen (p) + 1 bytes instead, so the NUL terminator moves with
the content it terminates, landing at the correct offset and leaving
no stale bytes in between.

Verified in an isolated harness extracting this exact code block: with
a reused buffer whose tail (beyond the true line length) still held
data from a previous iteration, the unfixed code returned 8 bytes
including that leftover tail; the fixed code returns exactly the
intended 6.

I was not able to build the full glibc tree in the environment I
found this in (glibc's build takes a base image / bootstrap toolchain
this sandbox doesn't have), so this is verified against the extracted
logic rather than the actual compiled __nss_readline, and I have not
run the existing nss test suite -- please double check with that as
part of review.

Signed-off-by: Ekalabya Ghosh <ekalabya2010@gmail.com>
---
nss/nss_readline.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

return 0;

Comments

Avinal Kumar Sept. 29, 2026, 7:04 a.m. UTC | #1 
On Sat, Sep 26, 2026 at 8:48 AM Ekalabya Ghosh <ekalabya2010@gmail.com> wrote:
>
> From: Ekalabya Ghosh <ekalabya2010@gmail.com>
> Date: Sat, 26 Sep 2026 08:47:45 +0530
> Subject: [PATCH] nss_readline: move the NUL terminator along with leading-whitespace strip
>
> __nss_readline() strips leading whitespace from a freshly-read
> database line by moving the remainder forward with
> "memmove (buf, p, strlen (p))". This moves the line's content but
> not its NUL terminator, which stays behind at its original, farther
> right position (the line's original, pre-strip length). The bytes
> between the newly shortened content and that stale terminator are the
> tail end of the original (pre-shift) line -- since the memmove only
> overwrites strlen (p) bytes starting at buf, positions at or beyond
> that offset keep whatever was already there, and the real terminator
> is further right than that. Reading the returned buffer back (e.g.
> via strlen) therefore includes those leftover bytes, appended after
> the line's own content.
>
> Concretely: for the two-space-indented line " hello\n" in an
> 8-plus-byte buffer, after stripping the leading whitespace the
> intended result is "hello\n" (6 bytes), but the buffer's terminator
> stays at its original offset (8), so bytes 6-7 -- which happen to
> still hold "o\n", the tail of the pre-strip line -- are included,
> and the caller sees "hello\no\n" (8 bytes) instead.
>
> This affects the last line of any nss_files-backed database
> (passwd, group, hosts, etc.) that has leading whitespace, appending
> extra, leftover bytes from the original line to the value the caller
> receives. It stays within the buffer (bounded by the earlier
> "buf[len - 1] == '\xff'" truncation check), so this is a
> data-correctness bug, not a memory-safety one.
>

Hi, I was able to reproduce the behavior. Although the practical
impact is low to none. The further processing pipeline is tolerant and
takes care of the trailing garbage. Can you please file a bug for the
same https://sourceware.org/bugzilla/enter_bug.cgi?product=glibc and
preferably attach a reproducer code? If you cannot do so, please let
me know I will open a bug on your behalf.

> I was not able to build the full glibc tree in the environment I
> found this in (glibc's build takes a base image / bootstrap toolchain
> this sandbox doesn't have), so this is verified against the extracted
> logic rather than the actual compiled __nss_readline, and I have not
> run the existing nss test suite -- please double check with that as
> part of review.
>

The patch doesn't apply. There are random line breaks and missing
space prefixes. I suspect the patch was copied using an HTML interface
and that messed up the formatting. Regenerate the patch using
git-format and use tools like git-send to submit.

> Signed-off-by: Ekalabya Ghosh <ekalabya2010@gmail.com>
> ---
> nss/nss_readline.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/nss/nss_readline.c b/nss/nss_readline.c
> index 08583e9d..fb0689cb 100644
> --- a/nss/nss_readline.c
> +++ b/nss/nss_readline.c
> @@ -71,7 +71,13 @@ __nss_readline (FILE *fp, char *buf, size_t len, off64_t *poffset)
> /* Skip empty lines and comments. */
> continue;
> if (p != buf)
> - memmove (buf, p, strlen (p));
> + /* Move the NUL terminator along with the line content. Moving
> + only strlen (p) bytes leaves the previous (now stale) NUL
> + terminator in its old, farther-right position, so the bytes
> + between the newly shortened line and that old terminator
> + -- leftover from the un-shifted tail of the original line --
> + are included in the result returned to the caller. */

The commit message already explains the rationale behind the fix.
Reduce the comment to just explain the change; something like "Include
the Null terminator" is enough.

> + memmove (buf, p, strlen (p) + 1);
>
A test case would strengthen this patch. A line with leading
whitespace in an NSS-format file fed through __nss_readline that
asserts the returned string length matches the expected stripped
content.

Thank
- Avinal
diff mbox series

Patch

diff --git a/nss/nss_readline.c b/nss/nss_readline.c
index 08583e9d..fb0689cb 100644
--- a/nss/nss_readline.c
+++ b/nss/nss_readline.c
@@ -71,7 +71,13 @@  __nss_readline (FILE *fp, char *buf, size_t len, off64_t
*poffset)
/* Skip empty lines and comments. */
continue;
if (p != buf)
- memmove (buf, p, strlen (p));
+ /* Move the NUL terminator along with the line content. Moving
+ only strlen (p) bytes leaves the previous (now stale) NUL
+ terminator in its old, farther-right position, so the bytes
+ between the newly shortened line and that old terminator
+ -- leftover from the un-shifted tail of the original line --
+ are included in the result returned to the caller. */
+ memmove (buf, p, strlen (p) + 1);

/* Return line to the caller. */