| Message ID | 20240702193343.5742-2-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] v4l-utils: fix formats under alpha/ppc64/mips64 | expand |
Hi Rosen, On 02/07/2024 21:33, Rosen Penev wrote: > Under musl, if a format string has an integer followed by %s, a mismatch > between types can cause the second half of the integer to be interpreted > by %s. > > Eg: printf("%d %s", 64bittype, string); > > will crash, especially on 32-bit big endian. > > The reason these are cast to __u64 is because time_t and suseconds_t > are 64-bit under musl, even on 32-bit platforms. __u64 helps avoid > any truncation issues that may or may not arise. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > utils/cec-follower/cec-follower.cpp | 2 +- > utils/libv4l2util/v4l2_driver.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp > index a7481aea..67e0d92b 100644 > --- a/utils/cec-follower/cec-follower.cpp > +++ b/utils/cec-follower/cec-follower.cpp > @@ -354,7 +354,7 @@ void print_timers(struct node *node) > printf("source: %s, ", source.c_str()); > if (t.recording_seq) > printf("rec-seq: 0x%x, ", t.recording_seq); > - printf("needs: %ld %s\n", t.duration, "MB."); /* 1MB per second. */ > + printf("needs: %llu %s\n", (__u64)t.duration, "MB."); /* 1MB per second. */ > } > printf("Total media space available for recording: "); > if (node->state.media_space_available >= 0) I will drop this change, but I will keep the next change. I'll explain more in my review of 4/4. Regards, Hans > diff --git a/utils/libv4l2util/v4l2_driver.c b/utils/libv4l2util/v4l2_driver.c > index 6b6366fa..51e97b61 100644 > --- a/utils/libv4l2util/v4l2_driver.c > +++ b/utils/libv4l2util/v4l2_driver.c > @@ -174,13 +174,13 @@ static void prt_buf_info(char *name,struct v4l2_buffer *p) > { > struct v4l2_timecode *tc=&p->timecode; > > - printf ("%s: %02ld:%02d:%02d.%08ld index=%d, type=%s, " > + printf ("%s: %02llu:%02d:%02d.%08llu index=%d, type=%s, " > "bytesused=%d, flags=0x%08x, " > "field=%s, sequence=%d, memory=%s, offset=0x%08x, length=%d\n", > - name, (p->timestamp.tv_sec/3600), > + name, (__u64)(p->timestamp.tv_sec/3600), > (int)(p->timestamp.tv_sec/60)%60, > (int)(p->timestamp.tv_sec%60), > - p->timestamp.tv_usec, > + (__u64)p->timestamp.tv_usec, > p->index, > prt_names(p->type,v4l2_type_names), > p->bytesused,p->flags,
On 19/07/2024 09:25, Hans Verkuil wrote: > Hi Rosen, > > On 02/07/2024 21:33, Rosen Penev wrote: >> Under musl, if a format string has an integer followed by %s, a mismatch >> between types can cause the second half of the integer to be interpreted >> by %s. >> >> Eg: printf("%d %s", 64bittype, string); >> >> will crash, especially on 32-bit big endian. >> >> The reason these are cast to __u64 is because time_t and suseconds_t >> are 64-bit under musl, even on 32-bit platforms. __u64 helps avoid >> any truncation issues that may or may not arise. >> >> Signed-off-by: Rosen Penev <rosenp@gmail.com> >> --- >> utils/cec-follower/cec-follower.cpp | 2 +- >> utils/libv4l2util/v4l2_driver.c | 6 +++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp >> index a7481aea..67e0d92b 100644 >> --- a/utils/cec-follower/cec-follower.cpp >> +++ b/utils/cec-follower/cec-follower.cpp >> @@ -354,7 +354,7 @@ void print_timers(struct node *node) >> printf("source: %s, ", source.c_str()); >> if (t.recording_seq) >> printf("rec-seq: 0x%x, ", t.recording_seq); >> - printf("needs: %ld %s\n", t.duration, "MB."); /* 1MB per second. */ >> + printf("needs: %llu %s\n", (__u64)t.duration, "MB."); /* 1MB per second. */ >> } >> printf("Total media space available for recording: "); >> if (node->state.media_space_available >= 0) > > I will drop this change, but I will keep the next change. > > I'll explain more in my review of 4/4. Actually, I decided to do this change differently by changing the duration type to int instead of using time_t. There is no reason for using time_t for this, since the duration is limited to 100*3600 seconds anyway. So that solves this particular issue without the need for a cast. I decided to accept 4/4 after all. I am a bit afraid that the same problem will reoccur if new printf statements are added showing timestamps, since this casting is quite fragile. But I don't really see a better approach. Regards, Hans > > Regards, > > Hans > >> diff --git a/utils/libv4l2util/v4l2_driver.c b/utils/libv4l2util/v4l2_driver.c >> index 6b6366fa..51e97b61 100644 >> --- a/utils/libv4l2util/v4l2_driver.c >> +++ b/utils/libv4l2util/v4l2_driver.c >> @@ -174,13 +174,13 @@ static void prt_buf_info(char *name,struct v4l2_buffer *p) >> { >> struct v4l2_timecode *tc=&p->timecode; >> >> - printf ("%s: %02ld:%02d:%02d.%08ld index=%d, type=%s, " >> + printf ("%s: %02llu:%02d:%02d.%08llu index=%d, type=%s, " >> "bytesused=%d, flags=0x%08x, " >> "field=%s, sequence=%d, memory=%s, offset=0x%08x, length=%d\n", >> - name, (p->timestamp.tv_sec/3600), >> + name, (__u64)(p->timestamp.tv_sec/3600), >> (int)(p->timestamp.tv_sec/60)%60, >> (int)(p->timestamp.tv_sec%60), >> - p->timestamp.tv_usec, >> + (__u64)p->timestamp.tv_usec, >> p->index, >> prt_names(p->type,v4l2_type_names), >> p->bytesused,p->flags, > >
diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp index a7481aea..67e0d92b 100644 --- a/utils/cec-follower/cec-follower.cpp +++ b/utils/cec-follower/cec-follower.cpp @@ -354,7 +354,7 @@ void print_timers(struct node *node) printf("source: %s, ", source.c_str()); if (t.recording_seq) printf("rec-seq: 0x%x, ", t.recording_seq); - printf("needs: %ld %s\n", t.duration, "MB."); /* 1MB per second. */ + printf("needs: %llu %s\n", (__u64)t.duration, "MB."); /* 1MB per second. */ } printf("Total media space available for recording: "); if (node->state.media_space_available >= 0) diff --git a/utils/libv4l2util/v4l2_driver.c b/utils/libv4l2util/v4l2_driver.c index 6b6366fa..51e97b61 100644 --- a/utils/libv4l2util/v4l2_driver.c +++ b/utils/libv4l2util/v4l2_driver.c @@ -174,13 +174,13 @@ static void prt_buf_info(char *name,struct v4l2_buffer *p) { struct v4l2_timecode *tc=&p->timecode; - printf ("%s: %02ld:%02d:%02d.%08ld index=%d, type=%s, " + printf ("%s: %02llu:%02d:%02d.%08llu index=%d, type=%s, " "bytesused=%d, flags=0x%08x, " "field=%s, sequence=%d, memory=%s, offset=0x%08x, length=%d\n", - name, (p->timestamp.tv_sec/3600), + name, (__u64)(p->timestamp.tv_sec/3600), (int)(p->timestamp.tv_sec/60)%60, (int)(p->timestamp.tv_sec%60), - p->timestamp.tv_usec, + (__u64)p->timestamp.tv_usec, p->index, prt_names(p->type,v4l2_type_names), p->bytesused,p->flags,
Under musl, if a format string has an integer followed by %s, a mismatch between types can cause the second half of the integer to be interpreted by %s. Eg: printf("%d %s", 64bittype, string); will crash, especially on 32-bit big endian. The reason these are cast to __u64 is because time_t and suseconds_t are 64-bit under musl, even on 32-bit platforms. __u64 helps avoid any truncation issues that may or may not arise. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- utils/cec-follower/cec-follower.cpp | 2 +- utils/libv4l2util/v4l2_driver.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)