[Linuxptp-devel] [PATCH] clock: fix possible buffer overrun

[Petr Kulhavy]

This is not a fix of an actual issue rather than prevention of a potential issue.
On two places a fixed array size (different to the actual size) is used in snprintf.
Replace with sizeof(array)

Signed-off-by: Petr Kulhavy <***@jikos.cz>
---
clock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clock.c b/clock.c
index 629a160..b6afba9 100644
--- a/clock.c
+++ b/clock.c
@@ -1053,7 +1053,7 @@ struct clock *clock_create(enum clock_type type, struct config *config,
c->utc_timescale = 1;
}
} else if (phc_index >= 0) {
- snprintf(phc, 31, "/dev/ptp%d", phc_index);
+ snprintf(phc, sizeof(phc), "/dev/ptp%d", phc_index);
c->clkid = phc_open(phc);
if (c->clkid == CLOCK_INVALID) {
pr_err("Failed to open %s: %m", phc);
@@ -1589,7 +1589,7 @@ int clock_switch_phc(struct clock *c, int phc_index)
clockid_t clkid;
char phc[32];

- snprintf(phc, 31, "/dev/ptp%d", phc_index);
+ snprintf(phc, sizeof(phc), "/dev/ptp%d", phc_index);
clkid = phc_open(phc);
if (clkid == CLOCK_INVALID) {
pr_err("Switching PHC, failed to open %s: %m", phc);

[Keller, Jacob E]

-----Original Message-----
Sent: Tuesday, May 16, 2017 7:57 AM
Subject: [Linuxptp-devel] [PATCH] clock: fix possible buffer overrun
This is not a fix of an actual issue rather than prevention of a potential issue.
On two places a fixed array size (different to the actual size) is used in snprintf.
Replace with sizeof(array)

Makes sense to me.

Thanks,
Jake

---
clock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clock.c b/clock.c
index 629a160..b6afba9 100644
--- a/clock.c
+++ b/clock.c
@@ -1053,7 +1053,7 @@ struct clock *clock_create(enum clock_type type, struct
config *config,
c->utc_timescale = 1;
}
} else if (phc_index >= 0) {
- snprintf(phc, 31, "/dev/ptp%d", phc_index);
+ snprintf(phc, sizeof(phc), "/dev/ptp%d", phc_index);
c->clkid = phc_open(phc);
if (c->clkid == CLOCK_INVALID) {
pr_err("Failed to open %s: %m", phc);
@@ -1589,7 +1589,7 @@ int clock_switch_phc(struct clock *c, int phc_index)
clockid_t clkid;
char phc[32];
- snprintf(phc, 31, "/dev/ptp%d", phc_index);
+ snprintf(phc, sizeof(phc), "/dev/ptp%d", phc_index);
clkid = phc_open(phc);
if (clkid == CLOCK_INVALID) {
pr_err("Switching PHC, failed to open %s: %m", phc);
--
2.7.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Richard Cochran]

Post by Petr Kulhavy
This is not a fix of an actual issue rather than prevention of a potential issue.

No, your patch introduces an issue that wasn't there before.

Post by Petr Kulhavy
@@ -1589,7 +1589,7 @@ int clock_switch_phc(struct clock *c, int phc_index)
clockid_t clkid;
char phc[32];
- snprintf(phc, 31, "/dev/ptp%d", phc_index);
+ snprintf(phc, sizeof(phc), "/dev/ptp%d", phc_index);

You replaced length 31 with 32. The code uses 31 for a reason.


Thanks, but no thanks,

Richard

[Petr Kulhavy]

Did I miss something? What is the reason then?

Petr

Post by Richard Cochran

Post by Petr Kulhavy
This is not a fix of an actual issue rather than prevention of a potential issue.

No, your patch introduces an issue that wasn't there before.

Post by Petr Kulhavy
@@ -1589,7 +1589,7 @@ int clock_switch_phc(struct clock *c, int phc_index)
clockid_t clkid;
char phc[32];
- snprintf(phc, 31, "/dev/ptp%d", phc_index);
+ snprintf(phc, sizeof(phc), "/dev/ptp%d", phc_index);

You replaced length 31 with 32. The code uses 31 for a reason.
Thanks, but no thanks,
Richard

[Richard Cochran]

Post by Petr Kulhavy
Did I miss something? What is the reason then?

Never mind, this is snprintf() and not strncpy().

The original code was confused and misleading using 31 (but not really
incorrect either).

Thanks,
Richard

[Petr Kulhavy]

That's what I mean. The size in snprintf() includes the terminating 0.
That's why I replaced it with sizeof.
And that's why I wrote in the commit message that there is no actual
issue, rather than a potential.
... when someone changes the size of the buffer downwards.

Cheers
Petr

Post by Richard Cochran

Post by Petr Kulhavy
Did I miss something? What is the reason then?

Never mind, this is snprintf() and not strncpy().
The original code was confused and misleading using 31 (but not really
incorrect either).
Thanks,
Richard

[Keller, Jacob E]

-----Original Message-----
Sent: Tuesday, May 16, 2017 1:19 PM
Subject: Re: [Linuxptp-devel] [PATCH] clock: fix possible buffer overrun

Post by Petr Kulhavy
Did I miss something? What is the reason then?

Never mind, this is snprintf() and not strncpy().
The original code was confused and misleading using 31 (but not really
incorrect either).
Thanks,
Richard

in GCC7, snprintf would now complain about possible truncation if you used size 31.

Thanks,
Jake

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Richard Cochran]

Post by Petr Kulhavy
This is not a fix of an actual issue rather than prevention of a potential issue.
On two places a fixed array size (different to the actual size) is used in snprintf.
Replace with sizeof(array)

I merged this change, but I completely re-wrote the commit message.

The subject line was especially problematic for me, because it said
"fix possible buffer overrun" which sounds like a serious bug fix.
In fact, this is more of a "best practice" change.

Thanks,
Richard