Skip to contentSkip to conversation

ALSA: oxfw: some signedness bugs

PATCH Applied Dan Carpenter 3 participants 9 messages Dec 12, 2014 – Dec 15, 2014

Subsystems (3)
Sound Kernel janitors Firewire audio drivers and IEC 61883-1/6 packet streaming engine
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/2 ALSA: oxfw: some signedness bugs

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Signed-off-by Dan Carpenter <[email protected]>
sound/firewire/oxfw/oxfw-proc.c +1 -1
index 604808e..8ba4f9f2 100644
@@ -15,7 +15,7 @@ static void proc_read_formation(struct snd_info_entry *entry,
 	struct snd_oxfw_stream_formation formation, curr;
 	u8 *format;
 	char flag;
-	unsigned int i, err;
+	int i, err;
 
 	/* Show input. */
 	err = snd_oxfw_stream_get_current_formation(oxfw,
sound/firewire/oxfw/oxfw-stream.c +2 -1
index b77cf80..bda845a 100644
@@ -61,7 +61,8 @@ static int set_stream_format(struct snd_oxfw *oxfw, struct amdtp_stream *s,
 	u8 **formats;
 	struct snd_oxfw_stream_formation formation;
 	enum avc_general_plug_dir dir;
-	unsigned int i, err, len;
+	unsigned int len;
+	int i, err;
 
 	if (s == &oxfw->tx_stream) {
 		formats = oxfw->tx_stream_formats;
sound/firewire/oxfw/oxfw-pcm.c +3 -3
index 9bc556b..67ade07 100644
@@ -19,7 +19,7 @@ static int hw_rule_rate(struct snd_pcm_hw_params *params,
 		.min = UINT_MAX, .max = 0, .integer = 1
 	};
 	struct snd_oxfw_stream_formation formation;
-	unsigned int i, err;
+	int i, err;
 
 	for (i = 0; i < SND_OXFW_STREAM_FORMAT_ENTRIES; i++) {
 		if (formats[i] == NULL)

@@ -47,7 +47,7 @@ static int hw_rule_channels(struct snd_pcm_hw_params *params,
 	const struct snd_interval *r =
 		hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE);
 	struct snd_oxfw_stream_formation formation;
-	unsigned int i, j, err;
+	int i, j, err;
 	unsigned int count, list[SND_OXFW_STREAM_FORMAT_ENTRIES] = {0};
 
 	count = 0;

@@ -80,7 +80,7 @@ static int hw_rule_channels(struct snd_pcm_hw_params *params,
 static void limit_channels_and_rates(struct snd_pcm_hardware *hw, u8 **formats)
 {
 	struct snd_oxfw_stream_formation formation;
-	unsigned int i, err;
+	int i, err;
 
 	hw->channels_min = UINT_MAX;
 	hw->channels_max = 0;
maintainer
#1 lore raw
Quoted 8 lines · Dan Carpenter

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Signed-off-by Dan Carpenter <[email protected]>

Thanks, but I prefer to use 'unsigned int' for loop counter, like the other drivers. Would you give 'int' type just for err variables?

Regards

Takashi Sakamoto
[email protected]

author
#2 lore raw
Quoted 8 lines · Takashi Sakamoto
Quoted 7 lines · Dan Carpenter

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Quoted 4 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

Thanks, but I prefer to use 'unsigned int' for loop counter, like the other drivers.

Unthinking use of unsigned int every where is a kind of leprosy. It just makes the code hard to read and it causes bugs.

regards,
dan carpenter

maintainer
#3 lore raw
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Takashi Sakamoto
Quoted 7 lines · Dan Carpenter

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

Thanks, but I prefer to use 'unsigned int' for loop counter, like the other drivers.

Unthinking use of unsigned int every where is a kind of leprosy. It just makes the code hard to read and it causes bugs.

In the beginning of last year, I received a comment from Clemens to use 'unsigned int' type for loop counter instead of 'int' type. Since then, for ALSA firewire stack, I've been following his advice. (but actually I missed to OXFW driver...)

For consistency inner this stack, I request you to follow this.

Regards

Takashi Sakamoto
[email protected]

author
#4 lore raw
Quoted 18 lines · Takashi Sakamoto
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Takashi Sakamoto
Quoted 7 lines · Dan Carpenter

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

Thanks, but I prefer to use 'unsigned int' for loop counter, like the other drivers.

Unthinking use of unsigned int every where is a kind of leprosy. It just makes the code hard to read and it causes bugs.

Quoted 7 lines

In the beginning of last year, I received a comment from Clemens to use 'unsigned int' type for loop counter instead of 'int' type. Since then, for ALSA firewire stack, I've been following his advice. (but actually I missed to OXFW driver...)

For consistency inner this stack, I request you to follow this.

That's nonsense advice. How many bugs has it prevented? It causes a lot.

Whatever, just fix it and give me the reported-by tag.

regards,
dan carpenter

author
#5 lore raw
In reply to Takashi Sakamoto

Think about a for loop:

	for (i = 0; i < xxx; i++) {

If "i" is really so high that it goes negative you have a very serious issue whether it's signed or unsigned. If it's signed then the loop is a no-op but if it's unsigned then it corrupts memory.

That's not a win.

regards,
dan carpenter

#6 lore raw
In reply to Takashi Sakamoto

At Sat, 13 Dec 2014 19:14:46 +0900,

Quoted 19 lines · Takashi Sakamoto
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Takashi Sakamoto
Quoted 7 lines · Dan Carpenter

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

Thanks, but I prefer to use 'unsigned int' for loop counter, like the other drivers.

Unthinking use of unsigned int every where is a kind of leprosy. It just makes the code hard to read and it causes bugs.

Quoted 6 lines

In the beginning of last year, I received a comment from Clemens to use 'unsigned int' type for loop counter instead of 'int' type. Since then, for ALSA firewire stack, I've been following his advice. (but actually I missed to OXFW driver...)

For consistency inner this stack, I request you to follow this.

Note that Dan's suggestions are about the variables like "i" and "j". These are used normally for small loop counts, and they are int in a standard idiom. If they are declared as another type, you force reader's attention *unnecessarily*, and it decreases the readability (i.e. they have to read the loop code as somewhat special even if it's a normal loop). This is the only big drawback, and the rest merit/demerit are almost ignorable, IMO.

Of course, in some cases, a loop count might be better in unsigned. But then a different variable name should be used instead.

After all, this is rather a minor issue, almost a bikeshed topic, so I didn't care much while reviewing your patches, and I still don't care whether this fix patch will have int or unsigned for i. But, it'd be good if you keep this information in your mind, at least.

thanks,

Takashi

maintainer Reviewed-by
#7 lore raw
Quoted 23 lines · Takashi Iwai

At Sat, 13 Dec 2014 19:14:46 +0900,

Quoted 4 lines · Takashi Sakamoto
Quoted 2 lines · Dan Carpenter
Quoted 1 line · Takashi Sakamoto

On Dec 13 2014 04:27, Dan Carpenter wrote:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Signed-off-by Dan Carpenter <[email protected]>

Note that Dan's suggestions are about the variables like "i" and "j". These are used normally for small loop counts, and they are int in a standard idiom. If they are declared as another type, you force reader's attention *unnecessarily*, and it decreases the readability (i.e. they have to read the loop code as somewhat special even if it's a normal loop). This is the only big drawback, and the rest merit/demerit are almost ignorable, IMO.

Quoted 7 lines

Of course, in some cases, a loop count might be better in unsigned. But then a different variable name should be used instead.

After all, this is rather a minor issue, almost a bikeshed topic, so I didn't care much while reviewing your patches, and I still don't care whether this fix patch will have int or unsigned for i. But, it'd be good if you keep this information in your mind, at least.

Reviewed-by Takashi Sakamoto <[email protected]>

Thanks

Takashi Sakamoto
[email protected]

#8 lore raw

At Mon, 15 Dec 2014 02:01:18 +0900,

Quoted 28 lines · Takashi Sakamoto
Quoted 26 lines · Takashi Iwai

At Sat, 13 Dec 2014 19:14:46 +0900,

Quoted 4 lines · Takashi Sakamoto
Quoted 2 lines · Dan Carpenter
Quoted 1 line · Takashi Sakamoto

On Dec 13 2014 04:27, Dan Carpenter wrote:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

This code tends to use unsigned variables by default and it causes signedness bugs when we use negative variables for error handling. The "i" and "j" variables are used to iterated over small positive values and so they should be type "int". The "len" variable doesn't *need* to be signed but it should be signed to make the code easier to read and audit.

Signed-off-by Dan Carpenter <[email protected]>

Note that Dan's suggestions are about the variables like "i" and "j". These are used normally for small loop counts, and they are int in a standard idiom. If they are declared as another type, you force reader's attention *unnecessarily*, and it decreases the readability (i.e. they have to read the loop code as somewhat special even if it's a normal loop). This is the only big drawback, and the rest merit/demerit are almost ignorable, IMO.

Of course, in some cases, a loop count might be better in unsigned. But then a different variable name should be used instead.

Quoted 6 lines
Quoted 4 lines

After all, this is rather a minor issue, almost a bikeshed topic, so I didn't care much while reviewing your patches, and I still don't care whether this fix patch will have int or unsigned for i. But, it'd be good if you keep this information in your mind, at least.

Reviewed-by Takashi Sakamoto <[email protected]>

OK, merged now. Thanks.

Takashi